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/7] 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/7] 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/7] 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` From e7df158cc4bc56a2a5e25f02ec6385810e0c3af2 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Fri, 2 Oct 2026 09:21:38 -0700 Subject: [PATCH 4/7] Return errors from package_todo.yml writes instead of panicking `write_package_todo_to_disk` and `delete_package_todo_from_disk` unwrapped every IO result, so a failed write or delete panicked. `pks rm` rewrites or deletes a single pack's todo file and should report a failure like any other error, so both now return a `Result`, and `write_violations_to_disk` passes it up to `update`. An `update` that cannot write a todo file now prints the error and exits 2, where it used to panic and exit 101. The `File::create` before each write is gone too: `fs::write` creates the file itself. --- src/packs/checker.rs | 2 +- src/packs/package_todo.rs | 39 +++++++++++++++++++++++++-------------- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/packs/checker.rs b/src/packs/checker.rs index e10262e..fe0d1fc 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -452,7 +452,7 @@ pub(crate) fn update(configuration: &Configuration) -> anyhow::Result<()> { configuration, violations, recorded_violations, - ); + )?; println!("Successfully updated package_todo.yml files!"); Ok(()) diff --git a/src/packs/package_todo.rs b/src/packs/package_todo.rs index 94afedb..9c6de62 100644 --- a/src/packs/package_todo.rs +++ b/src/packs/package_todo.rs @@ -1,3 +1,4 @@ +use anyhow::Context; use rayon::prelude::{IntoParallelRefIterator, ParallelIterator}; use serde::{ser::SerializeMap, Deserialize, Serialize, Serializer}; use std::collections::{BTreeMap, HashMap, HashSet}; @@ -135,7 +136,7 @@ pub fn write_violations_to_disk( configuration: &Configuration, violations: HashSet, recorded_violations: &HashSet, -) { +) -> anyhow::Result<()> { debug!("Starting writing violations to disk"); // First we need to group the violations by the responsible pack, which today is always the referencing pack // Later if we change where a violation shows up, we should delegate to the checker @@ -168,7 +169,7 @@ pub fn write_violations_to_disk( package_todos_for_pack_name(violations_by_responsible_pack); let all_packs = &configuration.pack_set.packs; - all_packs.par_iter().for_each(|p| { + all_packs.par_iter().try_for_each(|p| { let package_todo = package_todos_by_pack_name.get(&p.name); match package_todo { Some(package_todo) => write_package_todo_to_disk( @@ -178,9 +179,10 @@ pub fn write_violations_to_disk( ), None => delete_package_todo_from_disk(p), } - }); + })?; debug!("Finished writing violations to disk"); + Ok(()) } fn serialize_package_todo( @@ -197,32 +199,35 @@ fn serialize_package_todo( header + &package_todo_yml } -fn write_package_todo_to_disk( +pub(crate) fn write_package_todo_to_disk( responsible_pack: &Pack, package_todo: &PackageTodo, packs_first_mode: bool, -) { +) -> anyhow::Result<()> { let package_todo_yml_absolute_filepath = responsible_pack .yml .parent() .unwrap() .join("package_todo.yml"); - if !package_todo_yml_absolute_filepath.exists() { - std::fs::File::create(&package_todo_yml_absolute_filepath).unwrap(); - } - let package_todo_yml = serialize_package_todo( &responsible_pack.name, package_todo, packs_first_mode, ); - std::fs::write(package_todo_yml_absolute_filepath, package_todo_yml) - .unwrap(); + std::fs::write(&package_todo_yml_absolute_filepath, package_todo_yml) + .with_context(|| { + format!( + "Failed to write {}", + package_todo_yml_absolute_filepath.display() + ) + }) } -fn delete_package_todo_from_disk(responsible_pack: &Pack) { +pub(crate) fn delete_package_todo_from_disk( + responsible_pack: &Pack, +) -> anyhow::Result<()> { let package_todo_yml_absolute_filepath = responsible_pack .yml .parent() @@ -230,9 +235,15 @@ fn delete_package_todo_from_disk(responsible_pack: &Pack) { .join("package_todo.yml"); if package_todo_yml_absolute_filepath.exists() { - // Delete package_todo_yml_absolute_filepath - std::fs::remove_file(package_todo_yml_absolute_filepath).unwrap(); + std::fs::remove_file(&package_todo_yml_absolute_filepath) + .with_context(|| { + format!( + "Failed to delete {}", + package_todo_yml_absolute_filepath.display() + ) + })?; } + Ok(()) } fn header(responsible_pack_name: &String, packs_first_mode: bool) -> String { From 2a4de4aff2d7dee6a2b10b6c433d0c8f9f5e23a8 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Fri, 2 Oct 2026 09:22:59 -0700 Subject: [PATCH 5/7] Move dependency_removal out of checker/ and rename it pack_list `pks rm` will use this text edit too, from outside the checker and on `ignored_dependencies` and `visible_to` as well as `dependencies`. Move it first, unchanged, so that the next commit's diff shows only what changes in it. The new name is for the `PackList` type that commit adds. --- src/packs.rs | 1 + src/packs/checker.rs | 4 ++-- src/packs/{checker/dependency_removal.rs => pack_list.rs} | 0 3 files changed, 3 insertions(+), 2 deletions(-) rename src/packs/{checker/dependency_removal.rs => pack_list.rs} (100%) diff --git a/src/packs.rs b/src/packs.rs index 4490dad..9f35af8 100644 --- a/src/packs.rs +++ b/src/packs.rs @@ -16,6 +16,7 @@ pub(crate) mod ignored; pub(crate) mod json; pub(crate) mod monkey_patch_detection; pub(crate) mod pack; +pub(crate) mod pack_list; pub(crate) mod parsing; pub(crate) mod raw_configuration; pub(crate) mod template; diff --git a/src/packs/checker.rs b/src/packs/checker.rs index fe0d1fc..55f56e3 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -3,7 +3,6 @@ mod dependency; pub(crate) mod layer; mod common_test; -mod dependency_removal; mod folder_privacy; pub(crate) mod pack_checker; mod privacy; @@ -14,6 +13,7 @@ use crate::packs::checker_configuration::CheckerType; // Internal imports use crate::packs::pack::write_pack_to_disk; use crate::packs::pack::Pack; +use crate::packs::pack_list; use crate::packs::package_todo; use crate::packs::Configuration; use crate::packs::SourceLocation; @@ -612,7 +612,7 @@ fn remove_reference_to_dependency( .context(format!("Failed to read pack {:?}", pack.yml)) })?; - match dependency_removal::remove_dependencies(&contents, dependency_names) { + match pack_list::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) diff --git a/src/packs/checker/dependency_removal.rs b/src/packs/pack_list.rs similarity index 100% rename from src/packs/checker/dependency_removal.rs rename to src/packs/pack_list.rs From 6b5f3a748b51ff4682711a1a9aed641e69889016 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Fri, 2 Oct 2026 09:24:48 -0700 Subject: [PATCH 6/7] Add `pks rm` to delete a pack and the references to it `pks rm packs/foo` deletes the pack's directory after removing it from other packs' `dependencies`, `ignored_dependencies` and `visible_to` lists, and dropping the violations their package_todo.yml files record on it. Deleting only the directory leaves `validate` failing on dependencies that name a missing pack (packwerk-extensions rejects such `visible_to` entries too), and `check` failing on stale todo entries. The lists are edited as text, with #60's edit-then-verify approach generalized from `dependencies` to all three lists, so comments and key order survive. On copies of two large apps, the package.yml files it touched changed only on the removed entries' lines. Rewriting them through serde instead reorders the keys of most package.yml files in both apps. A `PackList` names the three keys. An emptied `visible_to` becomes `visible_to: []` rather than disappearing, because that is what serializing the pack writes, so the text edit and the fallback rewrite agree. The steps that read, edit, verify and fall back move into `remove_from_package_yml`, which auto-correct now calls too. Its warning names the lists it could not edit and gives the path relative to the project root. Unless given `--force`, it refuses while other packs may still use the pack, because once the pack is gone `check` cannot see their references to it: - Other packs reference its constants. Each reference is listed as file:line:column and constant. - It has Ruby files under lib/, other than lib/tasks/, that the Zeitwerk resolver doesn't know because they are outside the autoload roots. Gem-style packs keep their code there. Without this check, a widely used gem pack in one of those apps was deleted without objection. The experimental parser reads definitions from every file, so the check is skipped with it. The root pack can't be removed, and neither can a pack with other packs nested inside it, even with `--force`, since the nested packs would go without their references being removed. Fixes #16. --- CHANGELOG.md | 23 ++ README.md | 1 + src/packs.rs | 9 + src/packs/checker.rs | 45 +-- src/packs/cli.rs | 15 + src/packs/pack_list.rs | 258 +++++++++++++-- src/packs/remover.rs | 253 +++++++++++++++ .../app_with_obsolete_pack/package.yml | 2 + .../packs/bar/app/services/bar.rb | 2 + .../packs/bar/package.yml | 4 + .../packs/bar/package_todo.yml | 14 + .../packs/baz/app/services/baz.rb | 2 + .../packs/baz/package.yml | 5 + .../packs/foo/app/services/foo.rb | 5 + .../packs/foo/package.yml | 6 + .../packs/foo/package_todo.yml | 20 ++ .../packs/old/app/services/old.rb | 3 + .../packs/old/package.yml | 1 + .../app_with_obsolete_pack/packwerk.yml | 1 + tests/rm_test.rs | 293 ++++++++++++++++++ 20 files changed, 901 insertions(+), 61 deletions(-) create mode 100644 src/packs/remover.rs create mode 100644 tests/fixtures/app_with_obsolete_pack/package.yml create mode 100644 tests/fixtures/app_with_obsolete_pack/packs/bar/app/services/bar.rb create mode 100644 tests/fixtures/app_with_obsolete_pack/packs/bar/package.yml create mode 100644 tests/fixtures/app_with_obsolete_pack/packs/bar/package_todo.yml create mode 100644 tests/fixtures/app_with_obsolete_pack/packs/baz/app/services/baz.rb create mode 100644 tests/fixtures/app_with_obsolete_pack/packs/baz/package.yml create mode 100644 tests/fixtures/app_with_obsolete_pack/packs/foo/app/services/foo.rb create mode 100644 tests/fixtures/app_with_obsolete_pack/packs/foo/package.yml create mode 100644 tests/fixtures/app_with_obsolete_pack/packs/foo/package_todo.yml create mode 100644 tests/fixtures/app_with_obsolete_pack/packs/old/app/services/old.rb create mode 100644 tests/fixtures/app_with_obsolete_pack/packs/old/package.yml create mode 100644 tests/fixtures/app_with_obsolete_pack/packwerk.yml create mode 100644 tests/rm_test.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index fda1f30..afa47b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,29 @@ ## Unreleased +### Features + +#### `pks rm` removes a pack + +`pks rm packs/foo` deletes the pack's directory, after removing every mention of +it from other packs: their `dependencies`, `ignored_dependencies` and +`visible_to` lists, and the violations their `package_todo.yml` files record on +it. A todo file with nothing left in it is deleted. The lists are edited in +place, so the rest of each `package.yml`, comments and key order included, is +unchanged. + +By default it won't remove a pack that other packs may still use. It first +looks for references from other packs to the pack's constants and lists any it +finds, because once the pack is gone `pks check` can no longer see them. It also +won't remove a pack with Ruby files under `lib/` that are outside the autoload +roots, since pks can't see references to the constants those files define. The +experimental parser reads definitions from every file, so with +`--experimental-parser` that check is skipped. `--force` removes the pack anyway +and leaves any remaining references for you to fix. + +The root pack can't be removed. Neither can a pack with other packs inside it, +until those are removed. + ### Fixes #### ERB comments no longer hide or invent references diff --git a/README.md b/README.md index 920b5a1..dd4d647 100644 --- a/README.md +++ b/README.md @@ -46,6 +46,7 @@ Usage: pks [OPTIONS] Commands: greet Just saying hi create Create a new pack + rm Delete a pack and remove references to it from other packs check Look for violations in the codebase check-contents Check file contents piped to stdin update Update package_todo.yml files with the current violations diff --git a/src/packs.rs b/src/packs.rs index 9f35af8..27beaa4 100644 --- a/src/packs.rs +++ b/src/packs.rs @@ -19,6 +19,7 @@ pub(crate) mod pack; pub(crate) mod pack_list; pub(crate) mod parsing; pub(crate) mod raw_configuration; +pub(crate) mod remover; pub(crate) mod template; pub(crate) mod text; pub mod walk_directory; @@ -76,6 +77,14 @@ pub fn create( Ok(()) } +pub fn remove( + configuration: &Configuration, + name: String, + force: bool, +) -> anyhow::Result<()> { + remover::remove(configuration, &name, force) +} + /// Determine whether to use colors based on the color choice fn color_mode_for(color: ColorChoice) -> text::ColorMode { match color { diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 55f56e3..f5bdbd3 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -11,9 +11,8 @@ 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::pack_list; +use crate::packs::pack_list::{self, PackList}; use crate::packs::package_todo; use crate::packs::Configuration; use crate::packs::SourceLocation; @@ -463,7 +462,11 @@ pub(crate) fn remove_unnecessary_dependencies( ) -> anyhow::Result<()> { let unnecessary_dependencies = get_unnecessary_dependencies(configuration)?; for (pack, dependency_names) in unnecessary_dependencies.iter() { - remove_reference_to_dependency(pack, dependency_names)?; + pack_list::remove_from_package_yml( + pack, + &[PackList::Dependencies], + dependency_names, + )?; } Ok(()) } @@ -603,41 +606,5 @@ fn get_checkers( ] } -fn remove_reference_to_dependency( - pack: &Pack, - dependency_names: &[String], -) -> anyhow::Result<()> { - let contents = std::fs::read_to_string(&pack.yml).map_err(|e| { - anyhow::Error::new(e) - .context(format!("Failed to read pack {:?}", pack.yml)) - })?; - - match pack_list::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)?; - } - } - - 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/cli.rs b/src/packs/cli.rs index 7501180..e2592de 100644 --- a/src/packs/cli.rs +++ b/src/packs/cli.rs @@ -84,6 +84,18 @@ enum Command { #[clap(about = "Create a new pack")] Create { name: String }, + #[clap( + about = "Delete a pack and remove references to it from other packs" + )] + Rm { + /// The pack to delete + pack: String, + + /// Delete the pack even if other packs may still use its constants + #[arg(short, long)] + force: bool, + }, + #[clap(about = "Look for violations in the codebase")] Check { /// Ignore recorded violations when reporting violations @@ -334,5 +346,8 @@ pub fn run() -> anyhow::Result<()> { packs::lint_package_yml_files(&configuration) } Command::Create { name } => packs::create(&configuration, name), + Command::Rm { pack, force } => { + packs::remove(&configuration, pack, force) + } } } diff --git a/src/packs/pack_list.rs b/src/packs/pack_list.rs index 49e6a85..18c18bb 100644 --- a/src/packs/pack_list.rs +++ b/src/packs/pack_list.rs @@ -1,34 +1,147 @@ -use crate::packs::pack::Pack; +use crate::packs::pack::{write_pack_to_disk, Pack}; -/// Returns `contents` with `names` removed from its top-level `dependencies:` -/// list, edited as text so comments, key order and line endings survive. +/// A top-level `package.yml` key whose value is a list of pack names. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum PackList { + Dependencies, + IgnoredDependencies, + VisibleTo, +} + +impl PackList { + pub(crate) const ALL: [PackList; 3] = [ + PackList::Dependencies, + PackList::IgnoredDependencies, + PackList::VisibleTo, + ]; + + pub(crate) fn key(self) -> &'static str { + match self { + PackList::Dependencies => "dependencies", + PackList::IgnoredDependencies => "ignored_dependencies", + PackList::VisibleTo => "visible_to", + } + } + + pub(crate) fn contains(self, pack: &Pack, name: &str) -> bool { + match self { + PackList::Dependencies => pack.dependencies.contains(name), + PackList::IgnoredDependencies => { + pack.ignored_dependencies.contains(name) + } + PackList::VisibleTo => pack + .visible_to + .as_ref() + .is_some_and(|visible_to| visible_to.contains(name)), + } + } + + fn remove(self, pack: &mut Pack, names: &[String]) { + let keep = |name: &String| !names.contains(name); + match self { + PackList::Dependencies => pack.dependencies.retain(keep), + PackList::IgnoredDependencies => { + pack.ignored_dependencies.retain(keep) + } + PackList::VisibleTo => { + if let Some(visible_to) = &mut pack.visible_to { + visible_to.retain(keep) + } + } + } + } +} + +/// Edits `pack`'s `package.yml` in place where possible. Otherwise rewrites it, +/// with a warning, because rewriting loses its comments and key order. +pub(crate) fn remove_from_package_yml( + pack: &Pack, + lists: &[PackList], + names: &[String], +) -> anyhow::Result<()> { + let contents = std::fs::read_to_string(&pack.yml).map_err(|e| { + anyhow::Error::new(e) + .context(format!("Failed to read pack {:?}", pack.yml)) + })?; + + match remove_from_lists(&contents, lists, 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 {} in {} in place, so the file \ + was rewritten and its comments were not preserved.", + describe(lists), + pack.relative_yml().display() + ); + let mut updated_pack = pack.clone(); + for list in lists { + list.remove(&mut updated_pack, names); + } + write_pack_to_disk(&updated_pack)?; + } + } + + Ok(()) +} + +fn describe(lists: &[PackList]) -> String { + let keys: Vec<&str> = lists.iter().map(|list| list.key()).collect(); + match keys.split_last() { + Some((last, [])) => format!("{} list", last), + Some((last, rest)) => format!("{} and {} lists", rest.join(", "), last), + None => String::from("lists"), + } +} + +/// Returns `contents` with `names` removed from each of `lists`, 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 +/// pack minus exactly `names`, which happens when a list uses a layout the /// line matcher does not recognise (flow style, multi-line scalars, ...). -pub(super) fn remove_dependencies( +fn remove_from_lists( contents: &str, + lists: &[PackList], names: &[String], ) -> Option { - let edited = remove_dependency_lines(contents, names); - is_exact_removal(contents, &edited, names).then_some(edited) + let edited = lists.iter().fold(contents.to_owned(), |edited, list| { + remove_list_lines(&edited, *list, names) + }); + is_exact_removal(contents, &edited, lists, names).then_some(edited) } -fn is_exact_removal(original: &str, edited: &str, names: &[String]) -> bool { +fn is_exact_removal( + original: &str, + edited: &str, + lists: &[PackList], + 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)); + for list in lists { + list.remove(&mut expected, names); + } 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 { +/// left, the key goes too, as serializing the pack would do. `visible_to` is +/// the exception: serializing writes it as `visible_to: []`, so that is what +/// the key becomes. +fn remove_list_lines( + contents: &str, + list: PackList, + 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. @@ -63,7 +176,7 @@ fn remove_dependency_lines(contents: &str, names: &[String]) -> String { // the key. out.append(&mut pending_comments); in_block = false; - } else if key_index.is_none() && is_dependencies_key(line) { + } else if key_index.is_none() && is_key(line, list.key()) { key_index = Some(out.len()); in_block = true; } @@ -71,20 +184,34 @@ fn remove_dependency_lines(contents: &str, names: &[String]) -> String { } out.append(&mut pending_comments); - if let Some(index) = key_index.filter(|_| kept_items == 0) { - out.remove(index); + match key_index.filter(|_| kept_items == 0) { + None => out.concat(), + Some(index) if list == PackList::VisibleTo => { + // `is_key` matched, so the line starts with `visible_to:`. + let after_key = &out[index][list.key().len() + 1..]; + format!( + "{}{}: []{}{}", + out[..index].concat(), + list.key(), + after_key, + out[index + 1..].concat() + ) + } + Some(index) => { + out.remove(index); + out.concat() + } } - 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:" +/// Matches the top-level key only: a nested key of the same name is indented. +fn is_key(line: &str, key: &str) -> bool { + strip_comment(line).trim_end().strip_suffix(':') == Some(key) } /// 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. +/// `remove_from_lists` catches any item that this misreads. fn item_value(item: &str) -> &str { let item = item.trim(); for quote in ['"', '\''] { @@ -105,12 +232,20 @@ fn strip_comment(text: &str) -> &str { #[cfg(test)] mod tests { - use super::remove_dependencies; + use super::{describe, remove_from_lists, PackList}; use pretty_assertions::assert_eq; - fn remove(contents: &str, names: &[&str]) -> Option { + fn remove_from( + contents: &str, + lists: &[PackList], + names: &[&str], + ) -> Option { let names: Vec = names.iter().map(|n| n.to_string()).collect(); - remove_dependencies(contents, &names) + remove_from_lists(contents, lists, &names) + } + + fn remove(contents: &str, names: &[&str]) -> Option { + remove_from(contents, &[PackList::Dependencies], names) } fn assert_removes(before: &str, names: &[&str], after: &str) { @@ -299,5 +434,84 @@ ignored_dependencies: remove("dependencies:\n- \"packs/\\u0061\"\n", &["packs/a"]), None ); + assert_eq!( + remove_from( + "visible_to: [packs/a, packs/b]\n", + &[PackList::VisibleTo], + &["packs/a"] + ), + None + ); + } + + #[test] + fn removes_a_name_from_every_list_at_once() { + let before = "\ +# Header. +enforce_visibility: true +dependencies: +- packs/a +- packs/b +ignored_dependencies: +# Deliberately ignored: a would cycle. +- packs/a +visible_to: +- packs/a +- packs/c +metadata: + visible_to: + - packs/a +"; + let after = "\ +# Header. +enforce_visibility: true +dependencies: +- packs/b +visible_to: +- packs/c +metadata: + visible_to: + - packs/a +"; + assert_eq!( + remove_from(before, &PackList::ALL, &["packs/a"]).as_deref(), + Some(after) + ); + } + + #[test] + fn empties_visible_to_instead_of_removing_it() { + let visible_to = &[PackList::VisibleTo]; + assert_eq!( + remove_from( + "visible_to:\n- packs/a\nlayer: utilities\n", + visible_to, + &["packs/a"] + ) + .as_deref(), + Some("visible_to: []\nlayer: utilities\n") + ); + assert_eq!( + remove_from( + "visible_to: # Only a.\r\n- packs/a\r\n", + visible_to, + &["packs/a"] + ) + .as_deref(), + Some("visible_to: [] # Only a.\r\n") + ); + } + + #[test] + fn describes_the_lists_in_the_fallback_warning() { + assert_eq!(describe(&[PackList::Dependencies]), "dependencies list"); + assert_eq!( + describe(&[PackList::Dependencies, PackList::VisibleTo]), + "dependencies and visible_to lists" + ); + assert_eq!( + describe(&PackList::ALL), + "dependencies, ignored_dependencies and visible_to lists" + ); } } diff --git a/src/packs/remover.rs b/src/packs/remover.rs new file mode 100644 index 0000000..6548752 --- /dev/null +++ b/src/packs/remover.rs @@ -0,0 +1,253 @@ +use std::{collections::HashSet, path::Path}; + +use anyhow::{bail, Context}; + +use super::{ + checker::reference::Reference, + get_zeitwerk_constant_resolver, + pack::Pack, + pack_list::{self, PackList}, + package_todo, + reference_extractor::get_all_references, + Configuration, +}; + +/// Deletes the pack's directory after removing it from other packs' lists and +/// the violations their `package_todo.yml` files record on it. +pub(crate) fn remove( + configuration: &Configuration, + name: &str, + force: bool, +) -> anyhow::Result<()> { + let pack = configuration.pack_set.for_pack(name)?; + if pack.name == "." { + bail!("The root pack cannot be removed"); + } + + let nested_packs = nested_packs(configuration, pack); + if !nested_packs.is_empty() { + bail!( + "`{}` contains other packs, which must be removed first: {}", + pack.name, + nested_packs.join(", ") + ); + } + + if !force { + refuse_if_in_use(configuration, pack)?; + } + + let mut other_packs: Vec<&Pack> = configuration + .pack_set + .packs + .iter() + .filter(|other| other.name != pack.name) + .collect(); + other_packs.sort_by(|a, b| a.name.cmp(&b.name)); + + let names = [pack.name.clone()]; + for other in other_packs { + let lists: Vec = PackList::ALL + .into_iter() + .filter(|list| list.contains(other, &pack.name)) + .collect(); + if !lists.is_empty() { + pack_list::remove_from_package_yml(other, &lists, &names)?; + let keys: Vec<&str> = lists.iter().map(|list| list.key()).collect(); + println!( + "Removed `{}` from {} ({})", + pack.name, + other.relative_yml().display(), + keys.join(", ") + ); + } + remove_recorded_violations(configuration, other, &pack.name)?; + } + + // Last, because a partial delete can take package.yml with it, and then + // `rm` could no longer find the pack to remove the references to it. + let directory = configuration.absolute_root.join(&pack.relative_path); + std::fs::remove_dir_all(&directory) + .with_context(|| format!("Failed to delete {}", directory.display()))?; + println!("Successfully removed `{}`!", pack.name); + + Ok(()) +} + +/// Refuses while other packs may still use the pack's constants, because once +/// the pack is gone `check` cannot see those references. +fn refuse_if_in_use( + configuration: &Configuration, + pack: &Pack, +) -> anyhow::Result<()> { + let references = references_from_other_packs(configuration, pack)?; + for reference in &references { + println!( + "{}:{}:{} references {}", + reference.relative_referencing_file, + reference.source_location.line, + reference.source_location.column, + reference.constant_name + ); + } + + let references = counted(references.len(), "reference"); + let unchecked_files = unchecked_lib_files(configuration, pack); + let unchecked = counted(unchecked_files, "Ruby file").map(|files| { + format!( + "cannot resolve constants defined in `{}` ({}), which is outside \ + the autoload roots, so it cannot tell whether other packs use \ + them", + pack.relative_path.join("lib").display(), + files + ) + }); + match (references, unchecked) { + (None, None) => Ok(()), + (Some(references), None) => bail!( + "Found {} from other packs to constants in `{}`. Remove them, or \ + pass `--force` to remove the pack anyway.", + references, + pack.name + ), + (None, Some(unchecked)) => bail!( + "pks {}. Make sure none do, then pass `--force` to remove the pack.", + unchecked + ), + (Some(references), Some(unchecked)) => bail!( + "Found {} from other packs to constants in `{}`. pks also {}. \ + Remove the references and make sure nothing uses those \ + constants, then pass `--force` to remove the pack.", + references, + pack.name, + unchecked + ), + } +} + +/// `None` for zero, so callers can match on whether there is anything to say. +fn counted(count: usize, noun: &str) -> Option { + match count { + 0 => None, + 1 => Some(format!("1 {}", noun)), + _ => Some(format!("{} {}s", count, noun)), + } +} + +/// Counts the pack's Ruby files under `lib/` that define no constant pks knows. +/// The Zeitwerk resolver learns constants only from autoload roots, so +/// references to these files go unresolved. The experimental parser reads +/// every file, so it has no such gap. Nothing references Rake tasks in +/// `lib/tasks/` by constant. +fn unchecked_lib_files(configuration: &Configuration, pack: &Pack) -> usize { + if configuration.experimental_parser { + return 0; + } + let constant_resolver = get_zeitwerk_constant_resolver( + &configuration.pack_set, + &configuration.constant_resolver_configuration(), + ); + let definition_files: HashSet<&Path> = constant_resolver + .fully_qualified_constant_name_to_constant_definition_map() + .values() + .flatten() + .map(|definition| definition.absolute_path_of_definition.as_path()) + .collect(); + + let lib = configuration + .absolute_root + .join(&pack.relative_path) + .join("lib"); + let tasks = lib.join("tasks"); + configuration + .included_files + .iter() + .filter(|file| { + file.starts_with(&lib) + && !file.starts_with(&tasks) + && file.extension().is_some_and(|extension| extension == "rb") + && !definition_files.contains(file.as_path()) + }) + .count() +} + +fn nested_packs(configuration: &Configuration, pack: &Pack) -> Vec { + let mut nested: Vec = configuration + .pack_set + .packs + .iter() + .filter(|other| { + other.name != pack.name + && other.relative_path.starts_with(&pack.relative_path) + }) + .map(|other| other.name.clone()) + .collect(); + nested.sort(); + nested +} + +fn references_from_other_packs( + configuration: &Configuration, + pack: &Pack, +) -> anyhow::Result> { + let mut references: Vec = + get_all_references(configuration, &configuration.included_files)? + .into_iter() + .filter(|reference| { + reference.referencing_pack_name != pack.name + && reference.defining_pack_name.as_deref() + == Some(pack.name.as_str()) + }) + .collect(); + + references.sort_by(|a, b| location(a).cmp(&location(b))); + // A constant defined in several of the pack's files yields one per file. + references.dedup_by(|a, b| location(a) == location(b)); + + Ok(references) +} + +fn location(reference: &Reference) -> (&str, usize, usize, &str) { + ( + &reference.relative_referencing_file, + reference.source_location.line, + reference.source_location.column, + &reference.constant_name, + ) +} + +fn remove_recorded_violations( + configuration: &Configuration, + pack: &Pack, + removed: &str, +) -> anyhow::Result<()> { + let recorded = &pack.package_todo.violations_by_defining_pack; + if !recorded.contains_key(removed) { + return Ok(()); + } + + let package_todo_yml = pack.relative_path.join("package_todo.yml"); + if recorded.len() == 1 { + package_todo::delete_package_todo_from_disk(pack)?; + println!( + "Deleted {}, which only recorded violations on `{}`", + package_todo_yml.display(), + removed + ); + } else { + let mut package_todo = pack.package_todo.clone(); + package_todo.violations_by_defining_pack.remove(removed); + package_todo::write_package_todo_to_disk( + pack, + &package_todo, + configuration.packs_first_mode, + )?; + println!( + "Removed recorded violations on `{}` from {}", + removed, + package_todo_yml.display() + ); + } + + Ok(()) +} diff --git a/tests/fixtures/app_with_obsolete_pack/package.yml b/tests/fixtures/app_with_obsolete_pack/package.yml new file mode 100644 index 0000000..6b0f472 --- /dev/null +++ b/tests/fixtures/app_with_obsolete_pack/package.yml @@ -0,0 +1,2 @@ +dependencies: +- packs/old diff --git a/tests/fixtures/app_with_obsolete_pack/packs/bar/app/services/bar.rb b/tests/fixtures/app_with_obsolete_pack/packs/bar/app/services/bar.rb new file mode 100644 index 0000000..5003150 --- /dev/null +++ b/tests/fixtures/app_with_obsolete_pack/packs/bar/app/services/bar.rb @@ -0,0 +1,2 @@ +module Bar +end diff --git a/tests/fixtures/app_with_obsolete_pack/packs/bar/package.yml b/tests/fixtures/app_with_obsolete_pack/packs/bar/package.yml new file mode 100644 index 0000000..45e0b70 --- /dev/null +++ b/tests/fixtures/app_with_obsolete_pack/packs/bar/package.yml @@ -0,0 +1,4 @@ +enforce_visibility: true +visible_to: +- packs/foo +- packs/old diff --git a/tests/fixtures/app_with_obsolete_pack/packs/bar/package_todo.yml b/tests/fixtures/app_with_obsolete_pack/packs/bar/package_todo.yml new file mode 100644 index 0000000..3fde4eb --- /dev/null +++ b/tests/fixtures/app_with_obsolete_pack/packs/bar/package_todo.yml @@ -0,0 +1,14 @@ +# This file contains a list of dependencies that are not part of the long term plan for the +# 'packs/bar' package. +# We should generally work to reduce this list over time. +# +# You can regenerate this file using the following command: +# +# bin/packwerk update-todo +--- +packs/old: + "::Old": + violations: + - dependency + files: + - packs/bar/app/services/bar.rb diff --git a/tests/fixtures/app_with_obsolete_pack/packs/baz/app/services/baz.rb b/tests/fixtures/app_with_obsolete_pack/packs/baz/app/services/baz.rb new file mode 100644 index 0000000..dbe89a2 --- /dev/null +++ b/tests/fixtures/app_with_obsolete_pack/packs/baz/app/services/baz.rb @@ -0,0 +1,2 @@ +module Baz +end diff --git a/tests/fixtures/app_with_obsolete_pack/packs/baz/package.yml b/tests/fixtures/app_with_obsolete_pack/packs/baz/package.yml new file mode 100644 index 0000000..d175475 --- /dev/null +++ b/tests/fixtures/app_with_obsolete_pack/packs/baz/package.yml @@ -0,0 +1,5 @@ +enforce_visibility: true +ignored_dependencies: +- packs/old +visible_to: +- packs/old diff --git a/tests/fixtures/app_with_obsolete_pack/packs/foo/app/services/foo.rb b/tests/fixtures/app_with_obsolete_pack/packs/foo/app/services/foo.rb new file mode 100644 index 0000000..05ccb1a --- /dev/null +++ b/tests/fixtures/app_with_obsolete_pack/packs/foo/app/services/foo.rb @@ -0,0 +1,5 @@ +module Foo + def self.bar + Bar + end +end diff --git a/tests/fixtures/app_with_obsolete_pack/packs/foo/package.yml b/tests/fixtures/app_with_obsolete_pack/packs/foo/package.yml new file mode 100644 index 0000000..9b962d2 --- /dev/null +++ b/tests/fixtures/app_with_obsolete_pack/packs/foo/package.yml @@ -0,0 +1,6 @@ +# Foo's package. +enforce_dependencies: true +dependencies: +# Remove once Old is gone. +- packs/old +- packs/baz diff --git a/tests/fixtures/app_with_obsolete_pack/packs/foo/package_todo.yml b/tests/fixtures/app_with_obsolete_pack/packs/foo/package_todo.yml new file mode 100644 index 0000000..62badf1 --- /dev/null +++ b/tests/fixtures/app_with_obsolete_pack/packs/foo/package_todo.yml @@ -0,0 +1,20 @@ +# This file contains a list of dependencies that are not part of the long term plan for the +# 'packs/foo' package. +# We should generally work to reduce this list over time. +# +# You can regenerate this file using the following command: +# +# bin/packwerk update-todo +--- +packs/bar: + "::Bar": + violations: + - dependency + files: + - packs/foo/app/services/foo.rb +packs/old: + "::Old": + violations: + - privacy + files: + - packs/foo/app/services/foo.rb diff --git a/tests/fixtures/app_with_obsolete_pack/packs/old/app/services/old.rb b/tests/fixtures/app_with_obsolete_pack/packs/old/app/services/old.rb new file mode 100644 index 0000000..638f1a1 --- /dev/null +++ b/tests/fixtures/app_with_obsolete_pack/packs/old/app/services/old.rb @@ -0,0 +1,3 @@ +module Old + def self.call; end +end diff --git a/tests/fixtures/app_with_obsolete_pack/packs/old/package.yml b/tests/fixtures/app_with_obsolete_pack/packs/old/package.yml new file mode 100644 index 0000000..a473d63 --- /dev/null +++ b/tests/fixtures/app_with_obsolete_pack/packs/old/package.yml @@ -0,0 +1 @@ +enforce_privacy: true diff --git a/tests/fixtures/app_with_obsolete_pack/packwerk.yml b/tests/fixtures/app_with_obsolete_pack/packwerk.yml new file mode 100644 index 0000000..d7783cb --- /dev/null +++ b/tests/fixtures/app_with_obsolete_pack/packwerk.yml @@ -0,0 +1 @@ +cache: false diff --git a/tests/rm_test.rs b/tests/rm_test.rs new file mode 100644 index 0000000..7b34c26 --- /dev/null +++ b/tests/rm_test.rs @@ -0,0 +1,293 @@ +use assert_cmd::{assert::Assert, cargo::cargo_bin_cmd}; +use predicates::prelude::*; +use pretty_assertions::assert_eq; +use std::{collections::BTreeMap, error::Error, fs, path::Path}; + +mod common; + +// In the fixture, `packs/old` is about to be removed and every other pack +// still names it: the root and `packs/foo` depend on it, `packs/bar` and +// `packs/baz` list it in `visible_to`, `packs/baz` ignores it as a dependency, +// and the todo files of `packs/foo` and `packs/bar` record violations on it. +// No code references `Old` any more, so those todo entries are stale, as they +// are once the last use of a pack is deleted. + +fn pks(fixture: &common::Fixture, args: &[&str]) -> Assert { + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg(fixture.root()) + .args(args) + .assert() +} + +fn read(fixture: &common::Fixture, path: &str) -> String { + fs::read_to_string(fixture.path(path)) + .unwrap_or_else(|e| panic!("Could not read {}: {}", path, e)) +} + +fn snapshot(fixture: &common::Fixture) -> BTreeMap { + fn walk(root: &Path, dir: &Path, files: &mut BTreeMap) { + for entry in fs::read_dir(dir).unwrap() { + let path = entry.unwrap().path(); + if path.is_dir() { + walk(root, &path, files); + } else { + let relative = path.strip_prefix(root).unwrap(); + files.insert( + relative.display().to_string(), + fs::read_to_string(&path).unwrap(), + ); + } + } + } + let mut files = BTreeMap::new(); + walk(fixture.root(), fixture.root(), &mut files); + files +} + +fn reference_old_from_baz(fixture: &common::Fixture) { + fs::write( + fixture.path("packs/baz/app/services/baz.rb"), + "module Baz\n def self.old\n Old\n end\nend\n", + ) + .unwrap(); +} + +#[test] +fn test_rm() -> Result<(), Box> { + let fixture = common::Fixture::new("app_with_obsolete_pack"); + + pks(&fixture, &["rm", "packs/old"]) + .success() + .stdout( + "\ +Removed `packs/old` from ./package.yml (dependencies) +Removed `packs/old` from packs/bar/package.yml (visible_to) +Deleted packs/bar/package_todo.yml, which only recorded violations on `packs/old` +Removed `packs/old` from packs/baz/package.yml (ignored_dependencies, visible_to) +Removed `packs/old` from packs/foo/package.yml (dependencies) +Removed recorded violations on `packs/old` from packs/foo/package_todo.yml +Successfully removed `packs/old`! +", + ) + .stderr(""); + + assert!(!fixture.path("packs/old").exists()); + assert!(!fixture.path("packs/bar/package_todo.yml").exists()); + assert_eq!(read(&fixture, "package.yml"), ""); + // The comment above the removed entry goes with it. + assert_eq!( + read(&fixture, "packs/foo/package.yml"), + "\ +# Foo's package. +enforce_dependencies: true +dependencies: +- packs/baz +" + ); + assert_eq!( + read(&fixture, "packs/bar/package.yml"), + "enforce_visibility: true\nvisible_to:\n- packs/foo\n" + ); + assert_eq!( + read(&fixture, "packs/baz/package.yml"), + "enforce_visibility: true\nvisible_to: []\n" + ); + assert_eq!( + read(&fixture, "packs/foo/package_todo.yml"), + "\ +# This file contains a list of dependencies that are not part of the long term plan for the +# 'packs/foo' package. +# We should generally work to reduce this list over time. +# +# You can regenerate this file using the following command: +# +# bin/packwerk update-todo +--- +packs/bar: + \"::Bar\": + violations: + - dependency + files: + - packs/foo/app/services/foo.rb +" + ); + + pks(&fixture, &["validate"]).success(); + pks(&fixture, &["check"]).success(); + + Ok(()) +} + +#[test] +fn test_rm_accepts_a_trailing_slash() -> Result<(), Box> { + let fixture = common::Fixture::new("app_with_obsolete_pack"); + + pks(&fixture, &["rm", "packs/old/"]).success().stdout( + predicate::str::contains("Successfully removed `packs/old`!"), + ); + assert!(!fixture.path("packs/old").exists()); + + Ok(()) +} + +#[test] +fn test_rm_refuses_a_pack_other_packs_reference() -> Result<(), Box> +{ + let fixture = common::Fixture::new("app_with_obsolete_pack"); + reference_old_from_baz(&fixture); + let before = snapshot(&fixture); + + pks(&fixture, &["rm", "packs/old"]) + .code(2) + .stdout("packs/baz/app/services/baz.rb:3:4 references ::Old\n") + .stderr( + "Error: Found 1 reference from other packs to constants in \ + `packs/old`. Remove them, or pass `--force` to remove the pack \ + anyway.\n", + ); + + assert_eq!(snapshot(&fixture), before); + + Ok(()) +} + +#[test] +fn test_rm_force_removes_a_pack_other_packs_reference( +) -> Result<(), Box> { + let fixture = common::Fixture::new("app_with_obsolete_pack"); + reference_old_from_baz(&fixture); + + pks(&fixture, &["rm", "--force", "packs/old"]) + .success() + .stdout(predicate::str::contains( + "Successfully removed `packs/old`!", + )); + + assert!(!fixture.path("packs/old").exists()); + // `Old` no longer resolves, so the reference left in baz.rb goes unreported. + pks(&fixture, &["check"]).success(); + + Ok(()) +} + +#[test] +fn test_rm_refuses_a_pack_with_lib_code_pks_cannot_resolve( +) -> Result<(), Box> { + let fixture = common::Fixture::new("app_with_obsolete_pack"); + fs::create_dir_all(fixture.path("packs/old/lib/old"))?; + fs::write( + fixture.path("packs/old/lib/old/helper.rb"), + "module Old\n module Helper\n def self.help; end\n end\nend\n", + )?; + // Rake task support files are not counted. + fs::create_dir_all(fixture.path("packs/old/lib/tasks"))?; + fs::write(fixture.path("packs/old/lib/tasks/old.rb"), "")?; + let before = snapshot(&fixture); + + pks(&fixture, &["rm", "packs/old"]) + .code(2) + .stdout("") + .stderr( + "Error: pks cannot resolve constants defined in `packs/old/lib` (1 \ + Ruby file), which is outside the autoload roots, so it cannot tell \ + whether other packs use them. Make sure none do, then pass `--force` \ + to remove the pack.\n", + ); + assert_eq!(snapshot(&fixture), before); + + // The experimental parser reads definitions from every file, lib/ included. + pks(&fixture, &["--experimental-parser", "rm", "packs/old"]).success(); + assert!(!fixture.path("packs/old").exists()); + + Ok(()) +} + +#[test] +fn test_rm_refuses_with_both_reasons_at_once() -> Result<(), Box> { + let fixture = common::Fixture::new("app_with_obsolete_pack"); + reference_old_from_baz(&fixture); + fs::create_dir_all(fixture.path("packs/old/lib"))?; + fs::write(fixture.path("packs/old/lib/a.rb"), "")?; + fs::write(fixture.path("packs/old/lib/b.rb"), "")?; + + pks(&fixture, &["rm", "packs/old"]) + .code(2) + .stdout("packs/baz/app/services/baz.rb:3:4 references ::Old\n") + .stderr( + "Error: Found 1 reference from other packs to constants in \ + `packs/old`. pks also cannot resolve constants defined in \ + `packs/old/lib` (2 Ruby files), which is outside the autoload \ + roots, so it cannot tell whether other packs use them. Remove \ + the references and make sure nothing uses those constants, then \ + pass `--force` to remove the pack.\n", + ); + assert!(fixture.path("packs/old").exists()); + + Ok(()) +} + +#[test] +fn test_rm_refuses_the_root_pack() -> Result<(), Box> { + let fixture = common::Fixture::new("app_with_obsolete_pack"); + + pks(&fixture, &["rm", "--force", "."]) + .code(2) + .stderr("Error: The root pack cannot be removed\n"); + assert!(fixture.path("package.yml").exists()); + + Ok(()) +} + +#[test] +fn test_rm_refuses_a_pack_containing_other_packs() -> Result<(), Box> +{ + let fixture = common::Fixture::new("app_with_obsolete_pack"); + fs::create_dir_all(fixture.path("packs/old/nested"))?; + fs::write(fixture.path("packs/old/nested/package.yml"), "")?; + let before = snapshot(&fixture); + + // Not even with `--force`: references to the nested pack would be left. + pks(&fixture, &["rm", "--force", "packs/old"]) + .code(2) + .stderr( + "Error: `packs/old` contains other packs, which must be removed \ + first: packs/old/nested\n", + ); + assert_eq!(snapshot(&fixture), before); + + Ok(()) +} + +#[test] +fn test_rm_unknown_pack() -> Result<(), Box> { + let fixture = common::Fixture::new("app_with_obsolete_pack"); + + pks(&fixture, &["rm", "packs/nope"]) + .code(2) + .stderr("Error: No pack found 'packs/nope'\n"); + + Ok(()) +} + +#[test] +fn test_rm_rewrites_a_package_yml_it_cannot_edit_in_place( +) -> Result<(), Box> { + let fixture = common::Fixture::new("app_with_obsolete_pack"); + fs::write( + fixture.path("packs/foo/package.yml"), + "# Lost in the rewrite.\nenforce_dependencies: true\ndependencies: [packs/old, packs/baz]\n", + )?; + + pks(&fixture, &["rm", "packs/old"]).success().stderr( + "Warning: could not edit the dependencies list in \ + packs/foo/package.yml in place, so the file was rewritten and its \ + comments were not preserved.\n", + ); + assert_eq!( + read(&fixture, "packs/foo/package.yml"), + "enforce_dependencies: true\ndependencies:\n- packs/baz\n" + ); + + Ok(()) +} From 320accf3267faeb03303ad5ef2985b36013abf0a Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Fri, 2 Oct 2026 09:52:00 -0700 Subject: [PATCH 7/7] Bump version to 0.6.0 `pks rm` is a new command, and pre-1.0 a new feature wants a minor bump. Once this merges, auto-release.yml tags and releases v0.6.0, since Cargo.toml's version has no tag yet. Retitle `## Unreleased` to `## 0.6.0`, the heading dist takes the release notes from, and add entries for the two user-visible changes in this release that didn't have one: auto-correct keeping comments in package.yml (#60), using the entry #60 suggested, and the warm-cache speedup from skipping files that haven't changed (#59). --- CHANGELOG.md | 31 ++++++++++++++++++++++++++++++- Cargo.toml | 2 +- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b941184..73300ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## Unreleased +## 0.6.0 ### Features @@ -106,6 +106,35 @@ 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 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. + +### Performance + +#### A warm `pks check` skips reading files that haven't changed + +The cache now records each file's modification time and size along with its +digest, and serves a file whose time and size both still match without reading +it. On a 51,000-file application this made a warm `pks check` 1.43 times as +fast. When either has changed, pks reads the file and compares digests as +before. On a filesystem that records times only to the second, where a +same-size edit within a second can keep both, it never takes the shortcut. + +A tool that restores file times, such as `rsync -t`, `tar -p` or `cp -p`, can +put back different contents with the same size and time, and pks then serves +the old cached result. `touch` the file, or run with `--no-cache`, to check it +again. + ## 0.5.0 ### Breaking Changes diff --git a/Cargo.toml b/Cargo.toml index 2fc0835..d258361 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,7 +2,7 @@ [package] name = "pks" -version = "0.5.0" +version = "0.6.0" edition = "2021" description = "Welcome! Please see https://github.com/rubyatscale/pks for more information!" license = "MIT"