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/3] 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 ba9feff..1b7eb36 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 77be8bf..99265f8 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 7abbeb5..ada9a2f 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 bdb0ede..ad9202c 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/3] 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 cbed246..453e69d 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 1b7eb36..b84cc29 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 0000000..49e6a85 --- /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 99265f8..193788f 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/3] 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 453e69d..cbed246 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`