Skip to content

Stop lash format deleting sections and duplicating labels #44

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fohara merged 1 commit into from
Aug 9, 2026
Merged

Stop lash format deleting sections and duplicating labels #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions crates/lash-cli/src/commands/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,16 +347,17 @@ fn format_single_file(
let task_file = parse_file(file_path, config)
.with_context(|| format!("Failed to parse {}", file_path.display()))?;

// Read original content. The formatter needs it as well as the parsed
// file, to carry through sections the model does not represent.
let original = std::fs::read_to_string(file_path)
.with_context(|| format!("Failed to read {}", file_path.display()))?;

// Format the file
let formatter = Formatter::new(config.clone(), options.clone());
let formatted = formatter
.format_file(&task_file)
.format_file(&original, &task_file)
.with_context(|| format!("Failed to format {}", file_path.display()))?;

// Read original content
let original = std::fs::read_to_string(file_path)
.with_context(|| format!("Failed to read {}", file_path.display()))?;

// Check if content changed
let changed = formatted != original;

Expand Down
114 changes: 92 additions & 22 deletions crates/lash-core/src/formatter/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,13 @@
//! description: None,
//! description_agent_notes: Vec::new(),
//! };
//! let formatted = formatter.format_file(&file).unwrap();
//! let formatted = formatter.format_file("", &file).unwrap();
//! assert!(formatted.contains("# Test File"));
//! ```

pub mod options;

use crate::parser::header;
use lash_types::{LashConfig, Result, TaskFile, TaskStatus};
use std::path::Path;

Expand Down Expand Up @@ -125,41 +126,79 @@ impl Formatter {
/// # description: None,
/// # description_agent_notes: Vec::new(),
/// # };
/// let formatted = formatter.format_file(&file).unwrap();
/// let formatted = formatter.format_file("", &file).unwrap();
/// assert!(formatted.contains("# Test File"));
/// ```
#[allow(clippy::result_large_err)] // LashError is intentionally rich with context
pub fn format_file(&self, file: &TaskFile) -> Result<String> {
pub fn format_file(&self, source: &str, file: &TaskFile) -> Result<String> {
// Apply auto-fixes if enabled
let file = if self.options.apply_auto_fixes {
self.apply_auto_fixes(file)?
} else {
file.clone()
};

// Build the formatted output
let lines: Vec<&str> = source.lines().collect();
let header = header::header_span(source);
let description = header::section_span(source, "description");
let tasks = header::section_span(source, "tasks");

let mut output = String::new();

// 1. Format header (title, annotations, overview)
// The header is always regenerated, whether or not the source had one.
self.format_header(&file, &mut output);

// 2. Format description section (if present)
if file.description.is_some() {
Self::format_description(&file, &mut output);
}
// Then walk the rest of the source. Two spans are ours to replace with
// generated text; everything else is copied through byte for byte,
// because the model does not represent it and regenerating the file
// from the model alone silently deletes it.
// Each generated block ends with its own blank separator, so the
// source's is skipped rather than copied; emitting both would add a
// blank line on every run and formatting would not be idempotent.
let skip_blanks = |line: &mut usize| {
while lines.get(*line).is_some_and(|l| l.trim().is_empty()) {
*line += 1;
}
};

// 3. Format tasks section
self.format_tasks(&file, &mut output);
let mut line = header.end;
skip_blanks(&mut line);

// 4. Format references section (if present)
// TODO: Implement once references are stored in TaskFile
while line < lines.len() {
if description.as_ref().is_some_and(|span| span.start == line) {
Self::format_description(&file, &mut output);
line = description.as_ref().map_or(line + 1, |span| span.end);
skip_blanks(&mut line);
} else if tasks.as_ref().is_some_and(|span| span.start == line) {
self.format_tasks(&file, &mut output);
output.push('\n');
line = tasks.as_ref().map_or(line + 1, |span| span.end);
skip_blanks(&mut line);
} else {
output.push_str(lines[line]);
output.push('\n');
line += 1;
}
}

// 4. Normalize whitespace
// A source with no `## Tasks` heading still gets one. The parser treats
// such a file as all-tasks and warns, and emitting the section is what
// makes `lash format` able to repair it.
if tasks.is_none() {
if description.is_none() && file.description.is_some() {
Self::format_description(&file, &mut output);
}
self.format_tasks(&file, &mut output);
}

// Normalize whitespace. This only collapses runs of blank lines and
// trims trailing whitespace, so it is safe to run over copied-through
// content as well as generated content.
if self.options.normalize_whitespace {
output = self.normalize_whitespace(&output);
}

// 5. Ensure file ends with single newline
// Ensure file ends with single newline
if !output.ends_with('\n') {
output.push('\n');
} else if output.ends_with("\n\n") {
Expand Down Expand Up @@ -206,11 +245,18 @@ impl Formatter {
/// ```
#[allow(clippy::result_large_err)] // LashError is intentionally rich with context
pub fn format_file_in_place(&self, path: &Path) -> Result<()> {
// Parse the file
// Parse the file, keeping the source: the formatter needs it to carry
// through sections the model does not represent.
let source = std::fs::read_to_string(path).map_err(|e| lash_types::LashError::IO {
code: lash_types::error::codes::E_IO_READ_ERROR,
message: format!("Failed to read file for formatting: {e}"),
path: Some(path.to_path_buf()),
io_error: Some(e.to_string()),
})?;
let file = crate::parser::parse_file(path, &self.config)?;

// Format it
let formatted = self.format_file(&file)?;
let formatted = self.format_file(&source, &file)?;

// Write back atomically (tmp file + rename) so a crash mid-write
// can't leave a partial Markdown file on disk. This shares the same
Expand Down Expand Up @@ -525,11 +571,21 @@ impl Formatter {
output.push_str("- [");
output.push(task.status.to_checkbox_char());
output.push_str("] ");
output.push_str(&task.title);

// The parser leaves inline labels in the title *and* records them in
// metadata, so writing the title verbatim and then appending the
// labels emits each one twice. Formatting was therefore not
// idempotent: every run grew the line by another copy of every inline
// label, and `format --check` reported the file as needing formatting
// forever. Strip them from the title and let the canonical, sorted
// list below be the only place they appear.
output.push_str(&strip_inline_labels(&task.title));

// Add inline labels if present
if !task.metadata.labels.is_empty() {
for label in &task.metadata.labels {
let mut labels = task.metadata.labels.clone();
labels.sort();
for label in &labels {
output.push_str(" #");
output.push_str(label);
}
Expand Down Expand Up @@ -679,6 +735,20 @@ impl Formatter {
}
}

/// Remove inline `#label` words from a task title
///
/// The parser records inline labels in `TaskMetadata` without removing them
/// from the title, so a formatter that writes both would emit each label
/// twice. Only whole words that parsing would turn into labels are removed, so
/// a `#` inside prose survives.
fn strip_inline_labels(title: &str) -> String {
title
.split_whitespace()
.filter(|word| !lash_types::label::is_inline_label(word))
.collect::<Vec<_>>()
.join(" ")
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -779,7 +849,7 @@ mod tests {
make_task("task2", "Second task", TaskStatus::Done, None, 0),
]);

let result = formatter.format_file(&file).unwrap();
let result = formatter.format_file("", &file).unwrap();

assert!(result.contains("# Test File"));
assert!(result.contains("@id: test"));
Expand All @@ -800,7 +870,7 @@ mod tests {
make_task("child2", "Child 2", TaskStatus::Done, Some("parent"), 1),
]);

let result = formatter.format_file(&file).unwrap();
let result = formatter.format_file("", &file).unwrap();

assert!(result.contains("- [ ] Parent task"));
assert!(result.contains(" - [ ] Child 1"));
Expand All @@ -822,7 +892,7 @@ mod tests {
make_task("child2", "Child 2", TaskStatus::Open, Some("parent"), 1),
]);

let result = formatter.format_file(&file).unwrap();
let result = formatter.format_file("", &file).unwrap();

// Children should be auto-waived
assert!(result.contains("- [-] Parent"));
Expand Down
135 changes: 105 additions & 30 deletions crates/lash-core/src/parser/header.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,68 +348,143 @@ pub fn content_has_tasks_section(content: &str) -> bool {
false
}

/// Line span of the body of the `## Tasks` section
/// Line span of a `## <name>` section, heading line included
///
/// The range is 0-indexed and half-open: it starts at the first line after the
/// `## Tasks` heading and ends at the line holding the next H1 or H2 heading,
/// or at the end of the file. H3 and deeper headings sit inside the section
/// (files often group tasks under `### Subsection` headings) and do not close
/// it. Returns `None` if the file has no `## Tasks` heading.
/// The range is 0-indexed and half-open: it starts at the heading and ends at
/// the line holding the next H1 or H2 heading, or at the end of the file. H3
/// and deeper headings sit inside the section (files often group tasks under
/// `### Subsection` headings) and do not close it. Returns `None` if the file
/// has no such H2. Matching on `name` is case-insensitive.
///
/// A parsed [`lash_types::TaskFile`] records task line numbers but no section
/// boundaries, so anything that needs the extent of the Tasks section (in
/// particular appending to a section with no tasks in it yet) has to go back to
/// the source text. Heading detection runs through pulldown-cmark, so a `##`
/// inside a fenced code block does not close the section.
/// boundaries, so anything that needs the extent of a section has to go back
/// to the source text. Heading detection runs through pulldown-cmark, so a
/// `##` inside a fenced code block neither opens nor closes a section.
///
/// # Example
///
/// ```
/// use lash_core::parser::header::tasks_section_body;
/// use lash_core::parser::header::section_span;
///
/// let content = "# T\n\n## Tasks\n\n- [ ] One\n\n## Notes\n\nprose\n";
/// // Lines 3 through 5: the blank line, the task, and the blank line after it.
/// assert_eq!(tasks_section_body(content), Some(3..6));
///
/// // An empty section still has a body, it is just blank.
/// assert_eq!(tasks_section_body("# T\n\n## Tasks\n"), Some(3..3));
///
/// assert_eq!(tasks_section_body("# T\n\nno section here\n"), None);
/// assert_eq!(section_span(content, "tasks"), Some(2..6));
/// assert_eq!(section_span(content, "notes"), Some(6..9));
/// assert_eq!(section_span(content, "nope"), None);
/// ```
#[must_use]
pub fn tasks_section_body(content: &str) -> Option<std::ops::Range<usize>> {
pub fn section_span(content: &str, name: &str) -> Option<std::ops::Range<usize>> {
let mut heading_level: Option<HeadingLevel> = None;
let mut heading_text = String::new();
let mut body_start_byte: Option<usize> = None;
let mut body_end_byte = content.len();
let mut span: Option<std::ops::Range<usize>> = None;

for (event, range) in Parser::new(content).into_offset_iter() {
match event {
Event::Start(Tag::Heading { level, .. }) => {
if body_start_byte.is_some() && matches!(level, HeadingLevel::H1 | HeadingLevel::H2)
{
body_end_byte = range.start;
break;
if let Some(started) = span.as_mut() {
if matches!(level, HeadingLevel::H1 | HeadingLevel::H2) {
started.end = line_at_byte(content, range.start);
break;
}
}
heading_level = Some(level);
heading_text.clear();
}
Event::Text(text) if heading_level.is_some() => heading_text.push_str(&text),
Event::End(TagEnd::Heading(level)) => {
if body_start_byte.is_none()
if span.is_none()
&& level == HeadingLevel::H2
&& heading_text.trim().eq_ignore_ascii_case("tasks")
&& heading_text.trim().eq_ignore_ascii_case(name)
{
body_start_byte = Some(range.end);
let start = line_at_byte(content, range.start);
span = Some(start..content.lines().count());
}
heading_level = None;
}
_ => {}
}
}

let body_start_byte = body_start_byte?;
Some(line_at_byte(content, body_start_byte)..line_at_byte(content, body_end_byte))
span
}

/// Line span of the body of the `## Tasks` section
///
/// Same as [`section_span`] for "tasks", minus the heading line, so it starts
/// at the first line below `## Tasks`.
///
/// # Example
///
/// ```
/// use lash_core::parser::header::tasks_section_body;
///
/// let content = "# T\n\n## Tasks\n\n- [ ] One\n\n## Notes\n\nprose\n";
/// // Lines 3 through 5: the blank line, the task, and the blank line after it.
/// assert_eq!(tasks_section_body(content), Some(3..6));
///
/// // An empty section still has a body, it is just blank.
/// assert_eq!(tasks_section_body("# T\n\n## Tasks\n"), Some(3..3));
///
/// assert_eq!(tasks_section_body("# T\n\nno section here\n"), None);
/// ```
#[must_use]
pub fn tasks_section_body(content: &str) -> Option<std::ops::Range<usize>> {
section_span(content, "tasks").map(|span| (span.start + 1).min(span.end)..span.end)
}

/// Line span the formatter regenerates as the file header: the H1 and the
/// annotation block below it
///
/// Everything below it belongs to whoever is rewriting the file. Overview
/// prose and any section the model does not represent live there, and the
/// formatter has to copy them through rather than regenerate them.
///
/// The range always starts at 0. It is empty when the file has no H1, which is
/// the case where the parser synthesizes a title from the filename and the
/// formatter writes a header that was not there before.
///
/// # Example
///
/// ```
/// use lash_core::parser::header::header_span;
///
/// // The H1 and the `@id:` line, but not the prose below them.
/// assert_eq!(header_span("# T\n\n@id: t\n\nsome prose\n"), 0..3);
///
/// // No H1 to regenerate around.
/// assert_eq!(header_span("just prose\n"), 0..0);
/// ```
#[must_use]
pub fn header_span(content: &str) -> std::ops::Range<usize> {
let lines: Vec<&str> = content.lines().collect();

let Some(h1) = lines.iter().position(|line| {
let trimmed = line.trim_start();
trimmed.starts_with("# ") && !trimmed.starts_with("##")
}) else {
return 0..0;
};

let mut end = h1 + 1;
let mut saw_annotation = false;
for (offset, line) in lines.iter().enumerate().skip(h1 + 1) {
let trimmed = line.trim();
if trimmed.is_empty() {
// A blank line may separate the H1 from the annotations, or follow
// them. Either way it does not end the block on its own.
continue;
}
if trimmed.starts_with('@') {
saw_annotation = true;
end = offset + 1;
} else if saw_annotation && line.starts_with(' ') {
// Continuation of the annotation above.
end = offset + 1;
} else {
break;
}
}

0..end
}

/// 0-indexed line holding the given byte offset
Expand Down
Loading
Loading