From a556e0b6889d52356f31f0e9470616534bdf120f Mon Sep 17 00:00:00 2001 From: David Corson-Knowles Date: Wed, 23 Sep 2026 01:12:20 -0700 Subject: [PATCH 1/6] Preserve comments when auto-correcting unused dependencies `check-unused-dependencies --auto-correct` rebuilt the Pack struct and handed it to write_pack_to_disk, which re-serializes the whole package.yml through serde. serde has no notion of comments, so every comment in the file was silently deleted -- including comments documenting why an entry is in ignored_dependencies, which is exactly the kind of note a reader needs. It also rewrote the top-level keys into struct field order, which the existing test encoded as expected output (enforce_dependencies moved from first to third). Remove the offending list items as text instead. Reading the file, dropping the matching `- ` lines from the top-level `dependencies:` block, and writing it back leaves the rest byte for byte identical: comments in every position survive, key order is untouched, and other blocks -- notably ignored_dependencies -- are never considered. Verified against a real monorepo: injecting one unused dependency into a package.yml that carries two explanatory comments above an ignored_dependencies entry, then auto-correcting, now returns the file to byte-identical with its committed version. Before this change the two comments were gone. The fixture and test expectations grow comments in three positions -- above the first key, inside the dependencies block, and between the list and a following key -- so a regression here fails loudly. Note this fixes only the auto-correct path. write_pack_to_disk still destroys comments for callers that genuinely rewrite a pack (create, add-dependency, add-constant-dependencies); a general fix needs a comment-preserving YAML representation and is left out of scope. --- src/packs/checker.rs | 64 ++++++++++++++++--- tests/check_unused_dependencies.rs | 38 ++++++----- tests/common/mod.rs | 5 +- .../packs/foo/package.yml | 5 +- 4 files changed, 83 insertions(+), 29 deletions(-) diff --git a/src/packs/checker.rs b/src/packs/checker.rs index ba9feff3..1b7eb366 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -11,7 +11,6 @@ mod visibility; use crate::packs::checker_configuration::CheckerType; // Internal imports -use crate::packs::pack::write_pack_to_disk; use crate::packs::pack::Pack; use crate::packs::package_todo; use crate::packs::Configuration; @@ -475,16 +474,61 @@ fn remove_reference_to_dependency( pack: &Pack, dependency_names: &[String], ) -> anyhow::Result<()> { - let without_dependency = pack - .dependencies - .iter() - .filter(|dependency| !dependency_names.contains(dependency)); - let updated_pack = Pack { - dependencies: without_dependency.cloned().collect(), - ..pack.clone() - }; - write_pack_to_disk(&updated_pack)?; + // Edit the YAML as text rather than round-tripping the Pack struct. serde has no + // notion of comments, so re-serializing silently deletes every comment in the file + // and rewrites the top-level keys into struct order. Deleting just the offending + // list items leaves the rest of the file byte for byte identical. + let contents = std::fs::read_to_string(&pack.yml).map_err(|e| { + anyhow::Error::new(e) + .context(format!("Failed to read pack {:?}", pack.yml)) + })?; + + let updated = remove_dependency_lines(&contents, dependency_names); + + std::fs::write(&pack.yml, updated).map_err(|e| { + anyhow::Error::new(e) + .context(format!("Failed to write pack to disk {:?}", pack.yml)) + })?; + Ok(()) } + +/// Removes `- ` items for `dependency_names` from the top-level `dependencies:` +/// block, leaving comments, key order, and every other block (notably +/// `ignored_dependencies:`) untouched. +fn remove_dependency_lines( + contents: &str, + dependency_names: &[String], +) -> String { + let mut out: Vec<&str> = Vec::new(); + let mut in_dependencies = false; + + for line in contents.lines() { + if in_dependencies { + if let Some(item) = line.strip_prefix("- ") { + if dependency_names.iter().any(|name| name == item.trim()) { + continue; + } + out.push(line); + continue; + } + // A comment inside the block belongs to whatever follows it, so keep it and + // stay in the block. Anything else ends the block. + if !line.trim_start().starts_with('#') { + in_dependencies = false; + } + } else if line.trim_end() == "dependencies:" { + in_dependencies = true; + } + + out.push(line); + } + + let mut result = out.join("\n"); + if contents.ends_with('\n') { + result.push('\n'); + } + result +} // Note: Display impl was removed from CheckAllResult. Use write_text() directly with Configuration. // Tests for text formatting are in text.rs diff --git a/tests/check_unused_dependencies.rs b/tests/check_unused_dependencies.rs index 77be8bf3..99265f83 100644 --- a/tests/check_unused_dependencies.rs +++ b/tests/check_unused_dependencies.rs @@ -39,15 +39,16 @@ fn assert_auto_correct_unused_dependencies( ) -> Result<(), Box> { common::set_up_fixtures(); - let expected_before_autocorrect = [ - "enforce_dependencies: true", - "enforce_privacy: true", - "layer: technical_services", - "dependencies:", - "- packs/bar", - "- packs/baz\n", - ] - .join("\n"); + let expected_before_autocorrect = r#"# Header comment: proves comments above the first key survive. +enforce_dependencies: true +enforce_privacy: true +dependencies: +# Comment inside the dependencies block. +- packs/bar +- packs/baz +# Trailing comment, after the list and before another key. +layer: technical_services +"#; let foo_package_yml = fs::read_to_string("tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml").unwrap(); assert_eq!(foo_package_yml, expected_before_autocorrect); @@ -60,14 +61,17 @@ fn assert_auto_correct_unused_dependencies( .assert() .success(); - let expected_autocorrect = [ - "enforce_privacy: true", - "layer: technical_services", - "enforce_dependencies: true", - "dependencies:", - "- packs/bar\n", - ] - .join("\n"); + // Comments in all three positions survive, and the key order is unchanged: + // the correction deletes the one list item and nothing else. + let expected_autocorrect = r#"# Header comment: proves comments above the first key survive. +enforce_dependencies: true +enforce_privacy: true +dependencies: +# Comment inside the dependencies block. +- packs/bar +# Trailing comment, after the list and before another key. +layer: technical_services +"#; let after_autocorrect = fs::read_to_string("tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml").unwrap(); assert_eq!(after_autocorrect, expected_autocorrect); diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 7abbeb56..ada9a2ff 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -225,12 +225,15 @@ enforce_dependencies: true ); let pack_yml_contents = String::from( "\ +# Header comment: proves comments above the first key survive. enforce_dependencies: true enforce_privacy: true -layer: technical_services dependencies: +# Comment inside the dependencies block. - packs/bar - packs/baz +# Trailing comment, after the list and before another key. +layer: technical_services ", ); diff --git a/tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml b/tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml index bdb0edeb..ad9202cf 100644 --- a/tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml +++ b/tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml @@ -1,6 +1,9 @@ +# Header comment: proves comments above the first key survive. enforce_dependencies: true enforce_privacy: true -layer: technical_services dependencies: +# Comment inside the dependencies block. - packs/bar - packs/baz +# Trailing comment, after the list and before another key. +layer: technical_services From 30bc9d15817f222fea9898d835523f84307c38df Mon Sep 17 00:00:00 2001 From: David Corson-Knowles Date: Mon, 28 Sep 2026 19:37:54 -0700 Subject: [PATCH 2/6] Verify the text edit and fall back to rewriting when it can't be Review found that the line matcher only recognised unquoted, unindented items with nothing after them, and silently left every other dependency in place while exiting 0. Widen it and check its result: - accept indented lists, quoted items, a trailing `# comment` on an item or on the `dependencies:` key, and blank lines inside the list - parse the edited text back into a Pack and require it to equal the original minus exactly the removed dependencies; if it doesn't, fall back to write_pack_to_disk with a warning on stderr, so no layout that auto-correct handled before regresses - drop a comment directly above a removed item along with it - drop the `dependencies:` key when no items remain, as serializing did - keep line endings (split_inclusive) and skip the write when unchanged The editing moves into its own module with unit tests for each layout, and the integration tests now run check-unused-dependencies again after auto-correcting to assert it is clean. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 15 ++ src/packs/checker.rs | 72 ++---- src/packs/checker/dependency_removal.rs | 303 ++++++++++++++++++++++++ tests/check_unused_dependencies.rs | 88 ++++++- 4 files changed, 430 insertions(+), 48 deletions(-) create mode 100644 src/packs/checker/dependency_removal.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index cbed2469..453e69d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,21 @@ excluded, and the old behavior (analyze everything) was rarely desired. respect_gitignore: false ``` +### Fixes + +#### `check-unused-dependencies --auto-correct` keeps comments in `package.yml` + +Auto-correct used to rewrite every `package.yml` it touched from scratch, which +deleted all of its comments and reordered its keys. It now removes the unused +entries from the `dependencies:` list and leaves the rest of the file as it +was, line endings included. A comment directly above a removed entry is +removed with it. If no dependencies remain, the `dependencies:` key is +removed too, as before. + +If a list is written in a form that cannot be edited this way, such as +`dependencies: [packs/a, packs/b]`, the file is rewritten as before and a +warning is printed saying its comments were not preserved. + ### Internal #### Replaced `serde_yaml` with `yaml_serde` diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 1b7eb366..b84cc29a 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -3,6 +3,7 @@ mod dependency; pub(crate) mod layer; mod common_test; +mod dependency_removal; mod folder_privacy; pub(crate) mod pack_checker; mod privacy; @@ -11,6 +12,7 @@ mod visibility; use crate::packs::checker_configuration::CheckerType; // Internal imports +use crate::packs::pack::write_pack_to_disk; use crate::packs::pack::Pack; use crate::packs::package_todo; use crate::packs::Configuration; @@ -474,61 +476,37 @@ fn remove_reference_to_dependency( pack: &Pack, dependency_names: &[String], ) -> anyhow::Result<()> { - // Edit the YAML as text rather than round-tripping the Pack struct. serde has no - // notion of comments, so re-serializing silently deletes every comment in the file - // and rewrites the top-level keys into struct order. Deleting just the offending - // list items leaves the rest of the file byte for byte identical. let contents = std::fs::read_to_string(&pack.yml).map_err(|e| { anyhow::Error::new(e) .context(format!("Failed to read pack {:?}", pack.yml)) })?; - let updated = remove_dependency_lines(&contents, dependency_names); - - std::fs::write(&pack.yml, updated).map_err(|e| { - anyhow::Error::new(e) - .context(format!("Failed to write pack to disk {:?}", pack.yml)) - })?; - - Ok(()) -} - -/// Removes `- ` items for `dependency_names` from the top-level `dependencies:` -/// block, leaving comments, key order, and every other block (notably -/// `ignored_dependencies:`) untouched. -fn remove_dependency_lines( - contents: &str, - dependency_names: &[String], -) -> String { - let mut out: Vec<&str> = Vec::new(); - let mut in_dependencies = false; - - for line in contents.lines() { - if in_dependencies { - if let Some(item) = line.strip_prefix("- ") { - if dependency_names.iter().any(|name| name == item.trim()) { - continue; - } - out.push(line); - continue; - } - // A comment inside the block belongs to whatever follows it, so keep it and - // stay in the block. Anything else ends the block. - if !line.trim_start().starts_with('#') { - in_dependencies = false; - } - } else if line.trim_end() == "dependencies:" { - in_dependencies = true; + match dependency_removal::remove_dependencies(&contents, dependency_names) { + Some(updated) if updated == contents => {} + Some(updated) => std::fs::write(&pack.yml, updated).map_err(|e| { + anyhow::Error::new(e) + .context(format!("Failed to write pack to disk {:?}", pack.yml)) + })?, + None => { + eprintln!( + "Warning: could not edit the dependencies list in {} in \ + place, so the file was rewritten and its comments were \ + not preserved.", + pack.yml.display() + ); + let without_dependency = pack + .dependencies + .iter() + .filter(|dependency| !dependency_names.contains(dependency)); + let updated_pack = Pack { + dependencies: without_dependency.cloned().collect(), + ..pack.clone() + }; + write_pack_to_disk(&updated_pack)?; } - - out.push(line); } - let mut result = out.join("\n"); - if contents.ends_with('\n') { - result.push('\n'); - } - result + Ok(()) } // Note: Display impl was removed from CheckAllResult. Use write_text() directly with Configuration. // Tests for text formatting are in text.rs diff --git a/src/packs/checker/dependency_removal.rs b/src/packs/checker/dependency_removal.rs new file mode 100644 index 00000000..49e6a85b --- /dev/null +++ b/src/packs/checker/dependency_removal.rs @@ -0,0 +1,303 @@ +use crate::packs::pack::Pack; + +/// Returns `contents` with `names` removed from its top-level `dependencies:` +/// list, edited as text so comments, key order and line endings survive. +/// +/// Returns `None` when the edited text does not parse back to the original +/// pack minus exactly `names`, which happens when the list uses a layout the +/// line matcher does not recognise (flow style, multi-line scalars, ...). +pub(super) fn remove_dependencies( + contents: &str, + names: &[String], +) -> Option { + let edited = remove_dependency_lines(contents, names); + is_exact_removal(contents, &edited, names).then_some(edited) +} + +fn is_exact_removal(original: &str, edited: &str, names: &[String]) -> bool { + let (Ok(mut expected), Ok(actual)) = ( + yaml_serde::from_str::(original), + yaml_serde::from_str::(edited), + ) else { + return false; + }; + expected.dependencies.retain(|name| !names.contains(name)); + expected == actual +} + +/// Drops each matching item, along with any comment lines directly above it. +/// Other comments, blank lines and blocks are kept verbatim. If no items are +/// left, the `dependencies:` key goes too, as serializing the pack would do. +fn remove_dependency_lines(contents: &str, names: &[String]) -> String { + let mut out: Vec<&str> = Vec::new(); + // Comments seen inside the block, held until we know whether the item + // below them is kept. + let mut pending_comments: Vec<&str> = Vec::new(); + let mut key_index = None; + let mut in_block = false; + let mut kept_items = 0; + + for line in contents.split_inclusive('\n') { + if in_block { + let trimmed = line.trim_start(); + if let Some(item) = trimmed.strip_prefix("- ") { + if names.iter().any(|name| name == item_value(item)) { + pending_comments.clear(); + } else { + out.append(&mut pending_comments); + out.push(line); + kept_items += 1; + } + continue; + } + if trimmed.starts_with('#') { + pending_comments.push(line); + continue; + } + if trimmed.is_empty() { + out.append(&mut pending_comments); + out.push(line); + continue; + } + // A comment between the last item and the next key belongs to + // the key. + out.append(&mut pending_comments); + in_block = false; + } else if key_index.is_none() && is_dependencies_key(line) { + key_index = Some(out.len()); + in_block = true; + } + out.push(line); + } + out.append(&mut pending_comments); + + if let Some(index) = key_index.filter(|_| kept_items == 0) { + out.remove(index); + } + out.concat() +} + +/// Matches the top-level key only: a nested `dependencies:` is indented. +fn is_dependencies_key(line: &str) -> bool { + strip_comment(line).trim_end() == "dependencies:" +} + +/// The scalar value of a block list item, without quotes or a trailing +/// comment. Escapes inside quotes are not decoded; the parse check in +/// `remove_dependencies` catches any item that this misreads. +fn item_value(item: &str) -> &str { + let item = item.trim(); + for quote in ['"', '\''] { + if let Some(rest) = item.strip_prefix(quote) { + return rest.find(quote).map_or(item, |end| &rest[..end]); + } + } + strip_comment(item).trim_end() +} + +/// In YAML, `#` starts a comment only at the start of a line or after +/// whitespace, so `packs/a#b` is a plain value. +fn strip_comment(text: &str) -> &str { + text.match_indices('#') + .find(|(index, _)| *index == 0 || text[..*index].ends_with([' ', '\t'])) + .map_or(text, |(index, _)| &text[..index]) +} + +#[cfg(test)] +mod tests { + use super::remove_dependencies; + use pretty_assertions::assert_eq; + + fn remove(contents: &str, names: &[&str]) -> Option { + let names: Vec = names.iter().map(|n| n.to_string()).collect(); + remove_dependencies(contents, &names) + } + + fn assert_removes(before: &str, names: &[&str], after: &str) { + assert_eq!(remove(before, names).as_deref(), Some(after)); + } + + #[test] + fn removes_first_middle_and_last_items() { + let before = "dependencies:\n- packs/a\n- packs/b\n- packs/c\n"; + assert_removes( + before, + &["packs/a"], + "dependencies:\n- packs/b\n- packs/c\n", + ); + assert_removes( + before, + &["packs/b"], + "dependencies:\n- packs/a\n- packs/c\n", + ); + assert_removes( + before, + &["packs/c"], + "dependencies:\n- packs/a\n- packs/b\n", + ); + assert_removes( + before, + &["packs/a", "packs/c"], + "dependencies:\n- packs/b\n", + ); + } + + #[test] + fn removes_the_key_when_no_items_remain() { + assert_removes( + "enforce_dependencies: true\ndependencies:\n- packs/a\n- packs/b\nlayer: utilities\n", + &["packs/a", "packs/b"], + "enforce_dependencies: true\nlayer: utilities\n", + ); + assert_removes("dependencies:\n- packs/a\n", &["packs/a"], ""); + assert_removes( + "# Header.\ndependencies:\n- packs/a\n", + &["packs/a"], + "# Header.\n", + ); + } + + #[test] + fn handles_dependencies_as_the_last_key() { + assert_removes( + "enforce_privacy: true\ndependencies:\n- packs/a\n- packs/b\n", + &["packs/b"], + "enforce_privacy: true\ndependencies:\n- packs/a\n", + ); + assert_removes( + "enforce_privacy: true\ndependencies:\n- packs/a\n- packs/b", + &["packs/b"], + "enforce_privacy: true\ndependencies:\n- packs/a\n", + ); + } + + #[test] + fn handles_indented_lists() { + assert_removes( + "dependencies:\n - packs/a\n - packs/b\nlayer: utilities\n", + &["packs/a"], + "dependencies:\n - packs/b\nlayer: utilities\n", + ); + } + + #[test] + fn handles_quoted_items() { + assert_removes( + "dependencies:\n- \"packs/a\"\n- 'packs/b'\n- packs/c\n", + &["packs/a", "packs/b"], + "dependencies:\n- packs/c\n", + ); + } + + #[test] + fn handles_a_trailing_comment_on_an_item() { + assert_removes( + "dependencies:\n- packs/a # used\n- packs/b # unused\n", + &["packs/b"], + "dependencies:\n- packs/a # used\n", + ); + } + + #[test] + fn keeps_a_hash_that_is_part_of_the_value() { + assert_removes( + "dependencies:\n- packs/a#b\n- packs/a\n", + &["packs/a"], + "dependencies:\n- packs/a#b\n", + ); + } + + #[test] + fn handles_a_comment_on_the_key_line() { + assert_removes( + "dependencies: # keep sorted\n- packs/a\n- packs/b\n", + &["packs/b"], + "dependencies: # keep sorted\n- packs/a\n", + ); + } + + #[test] + fn handles_blank_lines_inside_the_list() { + assert_removes( + "dependencies:\n- packs/a\n\n- packs/b\n\nlayer: utilities\n", + &["packs/b"], + "dependencies:\n- packs/a\n\n\nlayer: utilities\n", + ); + } + + #[test] + fn removes_the_comment_directly_above_a_removed_item() { + assert_removes( + "dependencies:\n# Needed for A.\n- packs/a\n# Needed for B.\n- packs/b\n", + &["packs/a"], + "dependencies:\n# Needed for B.\n- packs/b\n", + ); + } + + #[test] + fn keeps_a_comment_separated_from_a_removed_item_by_a_blank_line() { + assert_removes( + "dependencies:\n# Keep sorted.\n\n- packs/a\n- packs/b\n", + &["packs/a"], + "dependencies:\n# Keep sorted.\n\n- packs/b\n", + ); + } + + #[test] + fn keeps_a_comment_between_the_list_and_the_next_key() { + assert_removes( + "dependencies:\n- packs/a\n- packs/b\n# About the layer.\nlayer: utilities\n", + &["packs/b"], + "dependencies:\n- packs/a\n# About the layer.\nlayer: utilities\n", + ); + } + + #[test] + fn leaves_ignored_dependencies_and_nested_keys_alone() { + assert_removes( + "\ +metadata: + dependencies: + - packs/a +dependencies: +- packs/a +- packs/b +ignored_dependencies: +# Deliberately ignored: a would cycle. +- packs/a +", + &["packs/a"], + "\ +metadata: + dependencies: + - packs/a +dependencies: +- packs/b +ignored_dependencies: +# Deliberately ignored: a would cycle. +- packs/a +", + ); + } + + #[test] + fn preserves_crlf_line_endings() { + assert_removes( + "# Header.\r\ndependencies:\r\n- packs/a\r\n- packs/b\r\n", + &["packs/b"], + "# Header.\r\ndependencies:\r\n- packs/a\r\n", + ); + } + + #[test] + fn declines_layouts_it_cannot_edit() { + assert_eq!( + remove("dependencies: [packs/a, packs/b]\n", &["packs/b"]), + None + ); + assert_eq!( + remove("dependencies:\n- \"packs/\\u0061\"\n", &["packs/a"]), + None + ); + } +} diff --git a/tests/check_unused_dependencies.rs b/tests/check_unused_dependencies.rs index 99265f83..193788f4 100644 --- a/tests/check_unused_dependencies.rs +++ b/tests/check_unused_dependencies.rs @@ -1,6 +1,6 @@ use assert_cmd::cargo::cargo_bin_cmd; use predicates::prelude::*; -use std::{error::Error, fs}; +use std::{error::Error, fs, path::Path}; mod common; fn assert_check_unused_dependencies(cmd: &str) -> Result<(), Box> { @@ -75,9 +75,44 @@ layer: technical_services let after_autocorrect = fs::read_to_string("tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml").unwrap(); assert_eq!(after_autocorrect, expected_autocorrect); + assert_no_unused_dependencies(Path::new( + "tests/fixtures/app_with_unnecessary_dependencies", + )); + Ok(()) } +fn assert_no_unused_dependencies(project_root: &Path) { + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg(project_root) + .arg("check-unused-dependencies") + .assert() + .success(); +} + +fn auto_correct_foo_package_yml(before: &str) -> (String, String) { + let fixture = common::Fixture::new("app_with_unnecessary_dependencies"); + let package_yml = fixture.path("packs/foo/package.yml"); + fs::write(&package_yml, before).unwrap(); + + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg(fixture.root()) + .arg("check-unused-dependencies") + .arg("--auto-correct") + .assert() + .success() + .get_output() + .clone(); + + assert_no_unused_dependencies(fixture.root()); + ( + fs::read_to_string(&package_yml).unwrap(), + String::from_utf8(output.stderr).unwrap(), + ) +} + #[test] fn test_auto_correct_unnecessary_dependencies() -> Result<(), Box> { assert_auto_correct_unused_dependencies( @@ -107,3 +142,54 @@ fn test_check_unnecessary_dependencies_no_issue() -> Result<(), Box> .success(); Ok(()) } + +#[test] +fn test_auto_correct_preserves_comments_in_hand_edited_layouts() { + let before = "\ +# Header comment. +enforce_dependencies: true +enforce_privacy: true +dependencies: # keep sorted + # Comment inside the dependencies block. + - \"packs/bar\" + + - packs/baz # unused +ignored_dependencies: + # Deliberately ignored: bop would cycle. + - packs/bop +"; + let expected = "\ +# Header comment. +enforce_dependencies: true +enforce_privacy: true +dependencies: # keep sorted + # Comment inside the dependencies block. + - \"packs/bar\" + +ignored_dependencies: + # Deliberately ignored: bop would cycle. + - packs/bop +"; + + let (after, stderr) = auto_correct_foo_package_yml(before); + assert_eq!(after, expected); + assert_eq!(stderr, ""); +} + +#[test] +fn test_auto_correct_falls_back_to_rewriting_layouts_it_cannot_edit() { + let before = "\ +# This comment is lost by the rewrite. +enforce_dependencies: true +enforce_privacy: true +dependencies: [packs/bar, packs/baz] +"; + + let (after, stderr) = auto_correct_foo_package_yml(before); + assert!(!after.contains("packs/baz"), "{after}"); + assert!(after.contains("- packs/bar"), "{after}"); + assert!( + stderr.contains("could not edit the dependencies list"), + "{stderr}" + ); +} From 1b71536deab055aaf7780660ed3f7f08bfcbf566 Mon Sep 17 00:00:00 2001 From: David Corson-Knowles Date: Mon, 28 Sep 2026 19:51:28 -0700 Subject: [PATCH 3/6] Leave the CHANGELOG entry to the merge The branch predates main's 0.4.0 and 0.5.0 releases, so an entry under its Unreleased heading conflicts with main. The entry text is in the PR description instead. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 453e69d8..cbed2469 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,21 +24,6 @@ excluded, and the old behavior (analyze everything) was rarely desired. respect_gitignore: false ``` -### Fixes - -#### `check-unused-dependencies --auto-correct` keeps comments in `package.yml` - -Auto-correct used to rewrite every `package.yml` it touched from scratch, which -deleted all of its comments and reordered its keys. It now removes the unused -entries from the `dependencies:` list and leaves the rest of the file as it -was, line endings included. A comment directly above a removed entry is -removed with it. If no dependencies remain, the `dependencies:` key is -removed too, as before. - -If a list is written in a form that cannot be edited this way, such as -`dependencies: [packs/a, packs/b]`, the file is rewritten as before and a -warning is printed saying its comments were not preserved. - ### Internal #### Replaced `serde_yaml` with `yaml_serde` From 9fe1f00277810dab9218fc490bd3cacb1c7fa5a7 Mon Sep 17 00:00:00 2001 From: David Corson-Knowles Date: Thu, 8 Oct 2026 22:18:43 -0700 Subject: [PATCH 4/6] Keep comments that still cover a kept entry and drop an emptied block whole A comment is now removed only when every entry below it, up to the next blank line or comment, is removed, and a commented-out entry is always kept. When no entries remain, the whole dependencies block goes, so its comments and blank lines no longer stay behind under the previous key. Removing an entry between blank lines no longer leaves two in a row. The fallback warning names the file relative to the project root and the dependencies removed, and says keys may be reordered. Adds the CHANGELOG entry. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 15 ++ src/packs/checker.rs | 14 +- src/packs/checker/dependency_removal.rs | 186 ++++++++++++++++++------ tests/check_unused_dependencies.rs | 18 ++- 4 files changed, 179 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c5c0759c..c6fe1ba7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,21 @@ longer found, so `pks check` reports it as stale until you run `pks update`. For the same reason, `check-unused-dependencies` may now report a dependency that only such an association used. packwerk still reports these references. +#### `check-unused-dependencies --auto-correct` keeps comments in `package.yml` + +Auto-correct used to rewrite every `package.yml` it touched from scratch, which +deleted all of its comments and reordered its keys. It now removes the unused +entries from the `dependencies:` list and leaves the rest of the file as it +was, line endings included. A comment covers the entries below it, up to the +next blank line or comment, and is removed only when all of those entries are, +unless it is a commented-out entry such as `# - packs/a`, which is always kept. +If no dependencies remain, the whole `dependencies:` block is removed, +including the comments and blank lines inside it. + +If a list is written in a form that cannot be edited this way, such as +`dependencies: [packs/a, packs/b]`, the file is rewritten as before and a +warning names the file and the dependencies removed. + ## 0.5.0 ### Breaking Changes diff --git a/src/packs/checker.rs b/src/packs/checker.rs index e10262e2..8344d505 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -613,17 +613,21 @@ fn remove_reference_to_dependency( })?; match dependency_removal::remove_dependencies(&contents, dependency_names) { - Some(updated) if updated == contents => {} Some(updated) => std::fs::write(&pack.yml, updated).map_err(|e| { anyhow::Error::new(e) .context(format!("Failed to write pack to disk {:?}", pack.yml)) })?, None => { eprintln!( - "Warning: could not edit the dependencies list in {} in \ - place, so the file was rewritten and its comments were \ - not preserved.", - pack.yml.display() + "Warning: could not edit the dependencies list in `{}` in \ + place to remove {}, so the file was rewritten: its comments \ + were not preserved and its keys may be reordered.", + pack.relative_yml().display(), + dependency_names + .iter() + .map(|name| format!("`{name}`")) + .collect::>() + .join(", "), ); let without_dependency = pack .dependencies diff --git a/src/packs/checker/dependency_removal.rs b/src/packs/checker/dependency_removal.rs index 49e6a85b..57e7c151 100644 --- a/src/packs/checker/dependency_removal.rs +++ b/src/packs/checker/dependency_removal.rs @@ -25,56 +25,117 @@ fn is_exact_removal(original: &str, edited: &str, names: &[String]) -> bool { expected == actual } -/// Drops each matching item, along with any comment lines directly above it. -/// Other comments, blank lines and blocks are kept verbatim. If no items are -/// left, the `dependencies:` key goes too, as serializing the pack would do. +/// Drops each matching item. A comment covers the items below it, up to the +/// next blank line or comment, and is dropped only when all of them are, +/// unless it holds a commented-out entry. If no items are left, the whole +/// block goes, as serializing the pack would do. Everything else is verbatim. fn remove_dependency_lines(contents: &str, names: &[String]) -> String { - let mut out: Vec<&str> = Vec::new(); - // Comments seen inside the block, held until we know whether the item - // below them is kept. - let mut pending_comments: Vec<&str> = Vec::new(); - let mut key_index = None; - let mut in_block = false; + let lines: Vec<&str> = contents.split_inclusive('\n').collect(); + let Some(key) = lines.iter().position(|line| is_dependencies_key(line)) + else { + return contents.to_string(); + }; + let mut block_end = key + 1; + while lines.get(block_end).is_some_and(|line| { + let trimmed = line.trim_start(); + trimmed.is_empty() + || trimmed.starts_with('#') + || trimmed.starts_with("- ") + }) { + block_end += 1; + } + // Comments directly above the next key belong to the key. + let mut tail = block_end; + while tail > key + 1 && lines[tail - 1].trim_start().starts_with('#') { + tail -= 1; + } + + let block = edit_block(&lines[key + 1..tail], names); + let mut out: Vec<&str> = lines[..key].to_vec(); + if let Some(block) = block { + out.push(lines[key]); + out.extend(block); + } + out.extend(&lines[tail..]); + out.concat() +} + +/// The block's lines with the named items removed, or `None` if no items are +/// left. +fn edit_block<'a>(lines: &[&'a str], names: &[String]) -> Option> { + let mut out: Vec<&'a str> = Vec::new(); + let mut group = Group::default(); let mut kept_items = 0; - for line in contents.split_inclusive('\n') { - if in_block { - let trimmed = line.trim_start(); - if let Some(item) = trimmed.strip_prefix("- ") { - if names.iter().any(|name| name == item_value(item)) { - pending_comments.clear(); - } else { - out.append(&mut pending_comments); - out.push(line); - kept_items += 1; - } - continue; - } - if trimmed.starts_with('#') { - pending_comments.push(line); - continue; - } - if trimmed.is_empty() { - out.append(&mut pending_comments); - out.push(line); - continue; - } - // A comment between the last item and the next key belongs to - // the key. - out.append(&mut pending_comments); - in_block = false; - } else if key_index.is_none() && is_dependencies_key(line) { - key_index = Some(out.len()); - in_block = true; + for &line in lines { + let trimmed = line.trim_start(); + if trimmed.starts_with("- ") { + group.items.push(line); + continue; + } + if trimmed.starts_with('#') && group.items.is_empty() { + group.comments.push(line); + continue; + } + let vanished = group.flush(&mut out, names, &mut kept_items); + if trimmed.starts_with('#') { + group.comments.push(line); + } else if !(vanished && out.last().is_some_and(|l| l.trim().is_empty())) + { + // Skipped when a removed group sat between two blank lines. + out.push(line); } - out.push(line); } - out.append(&mut pending_comments); + group.flush(&mut out, names, &mut kept_items); + (kept_items > 0).then_some(out) +} + +/// A run of comment lines and the items below them. +#[derive(Default)] +struct Group<'a> { + comments: Vec<&'a str>, + items: Vec<&'a str>, +} - if let Some(index) = key_index.filter(|_| kept_items == 0) { - out.remove(index); +impl<'a> Group<'a> { + /// Moves the group's surviving lines to `out`, returning whether it had + /// lines and none survived. + fn flush( + &mut self, + out: &mut Vec<&'a str>, + names: &[String], + kept_items: &mut usize, + ) -> bool { + let had_lines = !self.comments.is_empty() || !self.items.is_empty(); + let had_items = !self.items.is_empty(); + let kept: Vec<&'a str> = self + .items + .drain(..) + .filter(|line| !is_named_item(line, names)) + .collect(); + *kept_items += kept.len(); + let keep_comments = !had_items + || !kept.is_empty() + || self.comments.iter().any(|line| is_commented_out_item(line)); + if !keep_comments { + self.comments.clear(); + } + let vanished = had_lines && self.comments.is_empty() && kept.is_empty(); + out.append(&mut self.comments); + out.extend(kept); + vanished } - out.concat() +} + +fn is_named_item(line: &str, names: &[String]) -> bool { + let item = &line.trim_start()["- ".len()..]; + names.iter().any(|name| name == item_value(item)) +} + +fn is_commented_out_item(line: &str) -> bool { + line.trim_start() + .strip_prefix('#') + .is_some_and(|rest| rest.trim_start().starts_with("- ")) } /// Matches the top-level key only: a nested `dependencies:` is indented. @@ -221,7 +282,7 @@ mod tests { assert_removes( "dependencies:\n- packs/a\n\n- packs/b\n\nlayer: utilities\n", &["packs/b"], - "dependencies:\n- packs/a\n\n\nlayer: utilities\n", + "dependencies:\n- packs/a\n\nlayer: utilities\n", ); } @@ -289,6 +350,43 @@ ignored_dependencies: ); } + #[test] + fn keeps_a_comment_while_any_item_it_covers_remains() { + assert_removes( + "dependencies:\n# Keep this list sorted.\n- packs/baz\n- packs/bar\n", + &["packs/baz"], + "dependencies:\n# Keep this list sorted.\n- packs/bar\n", + ); + assert_removes( + "dependencies:\n# Keep this list sorted.\n- packs/baz\n- packs/bar\n\n- packs/bop\n", + &["packs/baz", "packs/bar"], + "dependencies:\n\n- packs/bop\n", + ); + } + + #[test] + fn keeps_a_commented_out_entry() { + assert_removes( + "dependencies:\n- packs/bar\n# - packs/bop # re-add after the migration\n- packs/baz\n", + &["packs/baz"], + "dependencies:\n- packs/bar\n# - packs/bop # re-add after the migration\n", + ); + } + + #[test] + fn removes_the_whole_block_when_no_items_remain() { + assert_removes( + "enforce_dependencies: true\ndependencies: # keep sorted\n - packs/a\n # Keep sorted.\n\nenforce_privacy: true\n", + &["packs/a"], + "enforce_dependencies: true\nenforce_privacy: true\n", + ); + assert_removes( + "metadata:\n notes: |\n Some notes.\ndependencies:\n # Needed for A.\n - packs/a\n\n # Needed for B.\n - packs/b\nlayer: utilities\n", + &["packs/a", "packs/b"], + "metadata:\n notes: |\n Some notes.\nlayer: utilities\n", + ); + } + #[test] fn declines_layouts_it_cannot_edit() { assert_eq!( diff --git a/tests/check_unused_dependencies.rs b/tests/check_unused_dependencies.rs index 193788f4..134fe44f 100644 --- a/tests/check_unused_dependencies.rs +++ b/tests/check_unused_dependencies.rs @@ -186,10 +186,18 @@ dependencies: [packs/bar, packs/baz] "; let (after, stderr) = auto_correct_foo_package_yml(before); - assert!(!after.contains("packs/baz"), "{after}"); - assert!(after.contains("- packs/bar"), "{after}"); - assert!( - stderr.contains("could not edit the dependencies list"), - "{stderr}" + let expected = "\ +enforce_privacy: true +enforce_dependencies: true +dependencies: +- packs/bar +"; + assert_eq!(after, expected); + assert_eq!( + stderr, + "Warning: could not edit the dependencies list in \ + `packs/foo/package.yml` in place to remove `packs/baz`, so the file \ + was rewritten: its comments were not preserved and its keys may be \ + reordered.\n" ); } From 1abcd1dde947a412b96198999103a7f1a43a2254 Mon Sep 17 00:00:00 2001 From: David Corson-Knowles Date: Sat, 17 Oct 2026 07:09:32 -0700 Subject: [PATCH 5/6] Make comment removal independent of the order entries are removed in Vetting the text edit over 775 real package.yml files, and synthetic variants of each (61,273 removals), turned up two cases where removing entries one at a time gave a different file than removing them together: - A commented-out entry left at the end of the list was treated as a comment above the next key, so when the list emptied it stayed behind, orphaned. Unindented comments above the next key still belong to it, but a commented-out entry now always belongs to the list. - A commented-out entry kept every comment in its run, including notes for entries below it that had all been removed. It now keeps only itself and the comments directly above it. Removing an entry between a blank line and either end of the list now takes the blank line with it. Tests cover each case, and one checks that every subset of a commented list gives the same result in either order. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 8 +- src/packs/checker/dependency_removal.rs | 149 +++++++++++++++++++++--- tests/check_unused_dependencies.rs | 1 - 3 files changed, 139 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c6fe1ba7..fba2a19f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -89,10 +89,10 @@ Auto-correct used to rewrite every `package.yml` it touched from scratch, which deleted all of its comments and reordered its keys. It now removes the unused entries from the `dependencies:` list and leaves the rest of the file as it was, line endings included. A comment covers the entries below it, up to the -next blank line or comment, and is removed only when all of those entries are, -unless it is a commented-out entry such as `# - packs/a`, which is always kept. -If no dependencies remain, the whole `dependencies:` block is removed, -including the comments and blank lines inside it. +next blank line or comment, and is removed only when all of those entries are. +A commented-out entry such as `# - packs/a` is always kept, along with any +comment directly above it. If no dependencies remain, the whole `dependencies:` +block is removed, including the comments and blank lines inside it. If a list is written in a form that cannot be edited this way, such as `dependencies: [packs/a, packs/b]`, the file is rewritten as before and a diff --git a/src/packs/checker/dependency_removal.rs b/src/packs/checker/dependency_removal.rs index 57e7c151..07471fbb 100644 --- a/src/packs/checker/dependency_removal.rs +++ b/src/packs/checker/dependency_removal.rs @@ -26,9 +26,10 @@ fn is_exact_removal(original: &str, edited: &str, names: &[String]) -> bool { } /// Drops each matching item. A comment covers the items below it, up to the -/// next blank line or comment, and is dropped only when all of them are, -/// unless it holds a commented-out entry. If no items are left, the whole -/// block goes, as serializing the pack would do. Everything else is verbatim. +/// next blank line or comment, and is dropped only when all of them are. A +/// commented-out entry, and any comment directly above one, is kept. If no +/// items are left, the whole block goes, as serializing the pack would do. +/// Everything else is verbatim. fn remove_dependency_lines(contents: &str, names: &[String]) -> String { let lines: Vec<&str> = contents.split_inclusive('\n').collect(); let Some(key) = lines.iter().position(|line| is_dependencies_key(line)) @@ -44,9 +45,13 @@ fn remove_dependency_lines(contents: &str, names: &[String]) -> String { }) { block_end += 1; } - // Comments directly above the next key belong to the key. + // Unindented comments directly above the next key belong to the key, + // except a commented-out entry, which belongs to the list. let mut tail = block_end; - while tail > key + 1 && lines[tail - 1].trim_start().starts_with('#') { + while tail > key + 1 + && lines[tail - 1].starts_with('#') + && !is_commented_out_item(lines[tail - 1]) + { tail -= 1; } @@ -80,13 +85,17 @@ fn edit_block<'a>(lines: &[&'a str], names: &[String]) -> Option> { let vanished = group.flush(&mut out, names, &mut kept_items); if trimmed.starts_with('#') { group.comments.push(line); - } else if !(vanished && out.last().is_some_and(|l| l.trim().is_empty())) + } else if !(vanished && out.last().is_none_or(|l| l.trim().is_empty())) { - // Skipped when a removed group sat between two blank lines. + // Skipped when a removed group sat between a blank line and + // another blank line or the start of the list. out.push(line); } } - group.flush(&mut out, names, &mut kept_items); + let vanished = group.flush(&mut out, names, &mut kept_items); + if vanished && out.last().is_some_and(|l| l.trim().is_empty()) { + out.pop(); + } (kept_items > 0).then_some(out) } @@ -114,11 +123,14 @@ impl<'a> Group<'a> { .filter(|line| !is_named_item(line, names)) .collect(); *kept_items += kept.len(); - let keep_comments = !had_items - || !kept.is_empty() - || self.comments.iter().any(|line| is_commented_out_item(line)); - if !keep_comments { - self.comments.clear(); + if had_items && kept.is_empty() { + // Keep commented-out entries and the comments above them. + let last_entry = self + .comments + .iter() + .rposition(|line| is_commented_out_item(line)); + self.comments + .truncate(last_entry.map_or(0, |index| index + 1)); } let vanished = had_lines && self.comments.is_empty() && kept.is_empty(); out.append(&mut self.comments); @@ -360,7 +372,7 @@ ignored_dependencies: assert_removes( "dependencies:\n# Keep this list sorted.\n- packs/baz\n- packs/bar\n\n- packs/bop\n", &["packs/baz", "packs/bar"], - "dependencies:\n\n- packs/bop\n", + "dependencies:\n- packs/bop\n", ); } @@ -397,5 +409,114 @@ ignored_dependencies: remove("dependencies:\n- \"packs/\\u0061\"\n", &["packs/a"]), None ); + assert_eq!( + remove("dependencies:\n-\tpacks/a\n- packs/b\n", &["packs/b"]), + None + ); + } + + #[test] + fn removes_a_trailing_commented_out_entry_with_the_block() { + assert_removes( + "dependencies:\n- packs/a\n# - packs/old\nlayer: utilities\n", + &["packs/a"], + "layer: utilities\n", + ); + assert_removes( + "dependencies:\n - packs/a\n # - packs/old\n", + &["packs/a"], + "", + ); + } + + #[test] + fn keeps_an_unindented_comment_at_the_end_of_the_file() { + assert_removes( + "dependencies:\n- packs/a\n# Footer.\n", + &["packs/a"], + "# Footer.\n", + ); + } + + #[test] + fn keeps_a_comment_directly_above_a_commented_out_entry() { + assert_removes( + "dependencies:\n- packs/a\n# Re-add after the migration:\n# - packs/old\n- packs/b\n", + &["packs/b"], + "dependencies:\n- packs/a\n# Re-add after the migration:\n# - packs/old\n", + ); + } + + #[test] + fn a_commented_out_entry_does_not_keep_a_comment_below_it() { + assert_removes( + "dependencies:\n- packs/a\n# - packs/old\n# Needed for B.\n- packs/b\n", + &["packs/b"], + "dependencies:\n- packs/a\n# - packs/old\n", + ); + } + + #[test] + fn removes_a_blank_line_left_at_either_end_of_the_list() { + assert_removes( + "dependencies:\n- packs/a\n\n- packs/b\n", + &["packs/a"], + "dependencies:\n- packs/b\n", + ); + assert_removes( + "dependencies:\n- packs/a\n\n- packs/b\nlayer: utilities\n", + &["packs/b"], + "dependencies:\n- packs/a\nlayer: utilities\n", + ); + } + + #[test] + fn leaves_document_markers_alone() { + assert_removes( + "---\ndependencies:\n- packs/a\n- packs/b\n...\n", + &["packs/a"], + "---\ndependencies:\n- packs/b\n...\n", + ); + } + + #[test] + fn removing_one_at_a_time_matches_removing_together() { + let before = "\ +enforce_dependencies: true +dependencies: +# Keep sorted. +- packs/a +- packs/b +# - packs/old +# Needed for C. +- packs/c # trailing + +# Needed for D and E. +- packs/d +- packs/e +# Re-add after the migration: +# - packs/older +- packs/f +# About the layer. +layer: utilities +"; + let all = [ + "packs/a", "packs/b", "packs/c", "packs/d", "packs/e", "packs/f", + ]; + for mask in 1..1u32 << all.len() { + let names: Vec<&str> = (0..all.len()) + .filter(|bit| mask & (1 << bit) != 0) + .map(|bit| all[bit]) + .collect(); + let together = remove(before, &names).unwrap(); + for order in [names.clone(), names.iter().rev().copied().collect()] + { + let one_at_a_time = + order.iter().fold(before.to_string(), |text, name| { + remove(&text, &[name]).unwrap() + }); + assert_eq!(one_at_a_time, together, "removing {order:?}"); + } + } } } diff --git a/tests/check_unused_dependencies.rs b/tests/check_unused_dependencies.rs index 134fe44f..ad9882a4 100644 --- a/tests/check_unused_dependencies.rs +++ b/tests/check_unused_dependencies.rs @@ -165,7 +165,6 @@ enforce_privacy: true dependencies: # keep sorted # Comment inside the dependencies block. - \"packs/bar\" - ignored_dependencies: # Deliberately ignored: bop would cycle. - packs/bop From 2c5ee4f2af1f16227fc6d104ad0daa540e0828cd Mon Sep 17 00:00:00 2001 From: David Corson-Knowles Date: Sat, 17 Oct 2026 07:53:15 -0700 Subject: [PATCH 6/6] Test inline comments on keys and values together Covers trailing comments on the dependencies key, on its items, and on the keys and items around it, for partial and full removal, through the CLI and the text edit. Also covers comments whose text names another pack or looks like an entry or a mapping, comments after a tab and before a CRLF, and empty comments. An anchored or tagged dependencies key falls back to rewriting. Co-Authored-By: Claude Opus 5.5 --- src/packs/checker/dependency_removal.rs | 82 +++++++++++++++++++++++++ tests/check_unused_dependencies.rs | 25 ++++++++ 2 files changed, 107 insertions(+) diff --git a/src/packs/checker/dependency_removal.rs b/src/packs/checker/dependency_removal.rs index 07471fbb..131a56be 100644 --- a/src/packs/checker/dependency_removal.rs +++ b/src/packs/checker/dependency_removal.rs @@ -289,6 +289,74 @@ mod tests { ); } + #[test] + fn handles_inline_comments_on_keys_and_values() { + let before = "\ +enforce_privacy: true # why +dependencies: # deps + - packs/a # used by A + - packs/b # unused +ignored_dependencies: # note + - packs/b # would cycle +layer: utilities # layer note +"; + assert_removes( + before, + &["packs/b"], + "\ +enforce_privacy: true # why +dependencies: # deps + - packs/a # used by A +ignored_dependencies: # note + - packs/b # would cycle +layer: utilities # layer note +", + ); + assert_removes( + before, + &["packs/a", "packs/b"], + "\ +enforce_privacy: true # why +ignored_dependencies: # note + - packs/b # would cycle +layer: utilities # layer note +", + ); + } + + #[test] + fn ignores_names_and_syntax_inside_inline_comments() { + assert_removes( + "dependencies: # a: b, [c]\n- packs/a # packs/b\n- packs/b # - packs/c\n- packs/c\n", + &["packs/b"], + "dependencies: # a: b, [c]\n- packs/a # packs/b\n- packs/c\n", + ); + assert_removes( + "dependencies:\n- \"packs/a\" # see \"packs/b\"\n- 'packs/b' # don't\n", + &["packs/b"], + "dependencies:\n- \"packs/a\" # see \"packs/b\"\n", + ); + assert_removes( + "dependencies: #\n- packs/a #\n- packs/b #\n", + &["packs/a"], + "dependencies: #\n- packs/b #\n", + ); + } + + #[test] + fn handles_inline_comments_after_tabs_and_before_crlf() { + assert_removes( + "dependencies:\t# k\n- packs/a\t# a\n- packs/b\t# b\n", + &["packs/b"], + "dependencies:\t# k\n- packs/a\t# a\n", + ); + assert_removes( + "dependencies: # k\r\n- packs/a # a\r\n- packs/b # b\r\n", + &["packs/b"], + "dependencies: # k\r\n- packs/a # a\r\n", + ); + } + #[test] fn handles_blank_lines_inside_the_list() { assert_removes( @@ -413,6 +481,20 @@ ignored_dependencies: remove("dependencies:\n-\tpacks/a\n- packs/b\n", &["packs/b"]), None ); + assert_eq!( + remove( + "dependencies: &deps # anchored\n- packs/a\n- packs/b\n", + &["packs/b"] + ), + None + ); + assert_eq!( + remove( + "dependencies: !!seq # tagged\n- packs/a\n- packs/b\n", + &["packs/b"] + ), + None + ); } #[test] diff --git a/tests/check_unused_dependencies.rs b/tests/check_unused_dependencies.rs index ad9882a4..789052ae 100644 --- a/tests/check_unused_dependencies.rs +++ b/tests/check_unused_dependencies.rs @@ -175,6 +175,31 @@ ignored_dependencies: assert_eq!(stderr, ""); } +#[test] +fn test_auto_correct_preserves_inline_comments_on_keys_and_values() { + let before = "\ +enforce_dependencies: true # enforced since the split +enforce_privacy: true # see the privacy docs +dependencies: # keep sorted +- packs/bar # used by Foo +- packs/baz # unused: was used by Foo's old job +ignored_dependencies: # cycles we accept for now +- packs/bop # would cycle with packs/baz +"; + let expected = "\ +enforce_dependencies: true # enforced since the split +enforce_privacy: true # see the privacy docs +dependencies: # keep sorted +- packs/bar # used by Foo +ignored_dependencies: # cycles we accept for now +- packs/bop # would cycle with packs/baz +"; + + let (after, stderr) = auto_correct_foo_package_yml(before); + assert_eq!(after, expected); + assert_eq!(stderr, ""); +} + #[test] fn test_auto_correct_falls_back_to_rewriting_layouts_it_cannot_edit() { let before = "\