Conversation
`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 `- <name>` 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.
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…pks-rm-subcommand
`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.
`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.
`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.
`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).
pks rm to delete a pack and the references to itpks rm and bump the version to 0.6.0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pks rm packs/foodeletes a pack along with what other packs say about it. It removes the pack from theirdependencies,ignored_dependenciesandvisible_tolists, drops the violations theirpackage_todo.ymlfiles record on it (deleting a todo file that ends up empty), and then deletes the pack's directory. Deleting the directory by hand leavespks validatefailing on dependencies that name a missing pack (packwerk-extensions rejects suchvisible_toentries too) andpks checkfailing on stale todo entries.Unless given
--force, it refuses to remove a pack that other packs may still use, because once the pack is gonecheckcan't see their references to it:file:line:columnand the constant.lib/(other thanlib/tasks/) that are outside the autoload roots, the Zeitwerk resolver never learns their constants, so references to them can't be checked. Gem-style packs keep their code there, and without this check a gem pack used across a large app 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.This PR also bumps the version to 0.6.0, so merging it releases v0.6.0.
Fixes #16.
Release
auto-release.ymldispatches the v0.6.0 release once this lands on main, sinceCargo.toml's version has no tag yet. 0.6.0 is a minor bump becausepks rmis a new feature.## Unreleasedin the CHANGELOG is now## 0.6.0, the heading dist takes the release notes from.package.yml(Preserve comments when auto-correcting unused dependencies #60, using the entry Preserve comments when auto-correcting unused dependencies #60 suggested) and the warm-cache speedup from skipping unchanged files (Skip reading source files on a warm cache hit via mtime + length #59).Stacked on #60
#60 is from a fork, so it can't be this PR's base. This branch merges #60's head instead, and until #60 lands the diff here includes it. The commits for review are the ones that aren't merges:
package_todo.ymlwrites return errors instead of panicking, sormcan report them.checker/dependency_removal.rstopack_list.rs, unchanged.rm.3d2061b merges main, for #68 and #69. Once #60 merges, I'll rebase onto main and drop both merges, along with the #60 CHANGELOG entry if #60 adds its own. Merging this PR first would ship #60 in 0.6.0, entry included, and #60 could then be closed.
Design notes
package.ymlfiles are edited as text, with Preserve comments when auto-correcting unused dependencies #60's edit-then-verify approach, so comments and key order survive. Rewriting through serde, asadd-dependencydoes, reorders keys: on copies of two large apps it changed mostpackage.ymlfiles even when nothing else changed, andrmcan touch many files at once.visible_tobecomesvisible_to: [], which is what serializing the pack writes, so the in-place edit and the fallback rewrite agree.pks updatewould also pick up any unrelated drift.package.ymlwith it, and thenrmcould no longer find the pack to clean up the references to it.Who this affects
pks rmis new.check-unused-dependencies --auto-correctnow sharesrm's editing code. Its fallback warning names the lists it couldn't edit and gives the path relative to the project root.pks updatereports a failure to write a todo file as an error and exits 2, where it used to panic.Test plan
tests/rm_test.rs: a removal that touches every kind of reference, checked byte for byte and followed by passingvalidateandcheck; refusals for references, for unresolvablelib/code, and for both at once, each leaving the fixture unchanged;--force; the root pack; nested packs; an unknown pack; a trailing slash; and the fallback rewrite.visible_to, and a flow-stylevisible_toit declines to edit.lib/tasks/exclusion or thevisible_to: []behavior fails tests.package.yml, and the todo files lost only the removed pack's blocks. Afterwardsvalidatepassed in both.checkmatched its baseline in one, and in the other reported the leftover references against the pack that defines an enclosing namespace, which is how the resolver already treats a missing constant.pks --versionprintspks 0.6.0, andCHANGELOG.mdhas the exact## 0.6.0heading thatauto-release.ymlchecks for.cargo test(354 passed, with Don't infer a constant from a polymorphic association #68 and Check the pks version before taking the cache's stat fast path #69 merged in), fmt, and clippy with-Dwarnings.