Repository navigation
Warn about files that don't parse, read <%== in ERB, and release 0.7.0 - #72
Merged
Merged
Conversation
`-p` printed "Started processing" and "Finished processing" lines to stdout, so the JSON from `check -o json` and `list-references` came after them and didn't parse. They are progress, not output, so they now go to stderr, where pks's warnings already go.
Rails' `<%== %>` outputs without escaping, but the regex that pulls Ruby out of ERB took only one `=` after `<%`, so `<%== foo(Bar) %>` became `= foo(Bar)`. That doesn't parse. lib-ruby-parser recovers only part of the file, and the tag's references and every reference after it were missed: with `<%= SomeConcern %>`, `<%== Bar.bar %>` and `<%= Baz %>`, pks found only `::SomeConcern`. In three large Rails apps, these were the only files that failed to parse. Packwerk reads ERB with a real parser and handles the tag.
dduugg
marked this pull request as ready for review
October 12, 2026 20:22
When lib-ruby-parser couldn't parse a file, pks used whatever partial tree it recovered, or none, and said nothing, so `check` could pass while missing the file's references. Packwerk reports a syntax error as an offense. pks now prints a warning to stderr with the file and the first error, and its line in Ruby. ERB gets no line, since its lines are those of the Ruby pks makes from it. The error rides on `ProcessedFile` as `syntax_error`, which isn't serialized, so the cache format that pks shares with packwerk doesn't change. A file pks warns about isn't written to the cache, so the warning comes back on every run until the file is fixed, instead of only when the file is first parsed. Only the app's own files, its included files, warn. `expose-monkey-patches` also parses Ruby's and the gems' sources, which can use syntax pks can't parse and aren't the app's to fix; they stay quiet and cached. An empty file has no AST but no error either, so it doesn't warn. Three large Rails apps give no warnings under either parser, now that `<%==` is read correctly. The parser can't read some newer syntax, such as Ruby 3.2's anonymous argument forwarding, and those files warn too.
`--referencing-pack` filtered references after pks had parsed every included file, so asking about one pack cost as much as asking about all of them. Only a pack's own files can make references from it, so pks now passes just those to `get_all_references`, as `check <paths>` passes its paths. The experimental parser still reads every file for definitions inside `get_all_references`, so it gains less. For two packs of a large Rails app, the output is the same as the full list limited to those packs, under both parsers, and the run took 2.6 s instead of 8.0 s with the default parser. A test checks that a file in another pack isn't read: one that doesn't parse would make pks warn.
pks's logger reads `RUST_LOG` even without `--debug`, so a developer with `RUST_LOG=debug` set saw every test that expects an empty stderr fail: 7 in `rm_test.rs`, all of `list_references_test.rs` and 5 in `check_test.rs`. CI doesn't set it, so only local runs were affected. `rm_test.rs` and `list_references_test.rs` each had the same `pks()` helper, and `list_references_test.rs` and `check_test.rs` each had a schema validator. Both move to `tests/common`, where `pks()` removes `RUST_LOG`; the new syntax error tests use it too. The `check_test.rs` tests run against the fixtures in place rather than through `Fixture`, so those five remove it themselves. With `RUST_LOG=debug`, the suite now fails only on `test_gitignore_negation_patterns`, which depends on the global gitignore rather than on logging.
`rm`'s search for references into the pack it removes filtered, sorted and deduplicated the references the way `list-references` does, with its own copy of each step. `between_packs` now holds the filter and the sort, and `rm` asks it for the references to one pack from the others, then keeps one copy of each, as `list-references` merges them. It and `identity` move to `reference_extractor`, which both commands already use, rather than one command reaching into the other's module. The order `rm` prints references in is unchanged, since `identity` only adds the defining pack, which is the same for all of them. Nothing tested that `rm` lists a reference once when several of the pack's files define its constant, so a new test does, under the experimental parser. It passes before and after this change and fails without the deduplication.
#71 needed `rayon`'s requirement raised from 1.7.0 to 1.10.0 by hand, for `par_chunk_by`. Nothing else raises a requirement, so a new API is found missing only when the build breaks. This takes the bundler and github-actions setup from rubyatscale/packs-specification: monthly, all updates in one group per ecosystem, with a 7-day cooldown. Cargo.lock isn't tracked, and Dependabot's default strategy leaves a requirement alone when it already allows the new release, as `1.7.0` allows 1.12. `versioning-strategy: increase` raises the requirement to the newest release instead, so Cargo.toml keeps up with what CI builds. dist generates release.yml and pins its actions in dist-workspace.toml, so the actions update leaves that file out.
`pks list-references` 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.7.0, since Cargo.toml's version has no tag yet. Retitle `## Unreleased` to `## 0.7.0`, the heading dist takes the release notes from.
dduugg
force-pushed
the
list-references-follow-ups
branch
from
October 12, 2026 20:24
07336b2 to
8637fbd
Compare
This was referenced Oct 12, 2026
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
1 participant
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
Follow-ups from the review of #71, plus a Dependabot config and the 0.7.0 bump. Each commit stands alone and has its own explanation.
checkcould pass while missing the file's references.<%==is read as Ruby. The ERB-to-Ruby regex took one=, so Rails' unescaped-output tag became= foo(Bar), which doesn't parse. The tag's references and every reference after it in the file were missed.-pno longer breaks JSON.--print-filesprogress lines go to stderr, socheck -o json -pandlist-references -pproduce valid JSON.list-references --referencing-packreads only those packs' files, rather than parsing everything and filtering afterwards.rmshares list-references' reference selection (filter, sort, merge of copies), now inreference_extractor.pks()and the schema validator move totests/common, and tests that expect an empty stderr removeRUST_LOG, so they pass withRUST_LOG=debugset.list-referencesis a new command. Merging this releases v0.7.0 through auto-release.yml.Design notes
expose-monkey-patchesalso parses Ruby's and the gems' sources, which can use syntax pks can't parse and aren't the app's to fix, so they stay quiet and cached.syntax_errorisn't serialized, so the cache format pks shares with packwerk doesn't change.<%==files were the only ones that failed to parse. With that fixed, neither parser gives any warnings there.def foo(*) = bar(*)), so files using it now warn instead of silently losing references. lib-ruby-parser's repository is archived, and its last release is from April 2024. Moving toruby-prism, the official bindings to the parser Ruby itself uses, would fix this, and belongs in its own PR.versioning-strategy: increase.Cargo.lockisn't tracked. Dependabot's default strategy leaves a requirement alone when it already allows the new release, asrayon = "1.7.0"allows 1.12.increaseraises the floor inCargo.tomlinstead, which is what Addpks list-referencesto list the references between packs #71 had to do by hand forpar_chunk_by.release.ymlis left out of the Actions updates. dist generates it and pins its actions indist-workspace.toml.exclude-pathsis documented per update block, but I couldn't confirm that thegithub-actionsecosystem honors it. If it doesn't, Dependabot may propose edits torelease.ymlthat should be closed.Who this affects
<%==.checkcan now report violations it missed before, and those files print a warning.-pwith JSON output, orlist-references --referencing-packon a large app.Test plan
expose-monkey-patches;<%==;-pwith JSON output;--referencing-packnot reading other packs' files;rmlisting a reference once when several files define its constant. Each fails when the behavior it covers is removed.--referencing-packoutput matches the full list limited to those packs, under both parsers. With the default parser, the larger app took 2.6 s instead of 8.0 s.cargo fmt --checkandcargo clippy --all-targets --all-featureswith-Dwarnings. The suite passes with and withoutRUST_LOG=debug. zizmor finds nothing in the newdependabot.yml.