compare: address versions by slug and ordinal, not just by path - #426
Open
Nitjsefnie wants to merge 6 commits into
Open
compare: address versions by slug and ordinal, not just by path#426Nitjsefnie wants to merge 6 commits into
Nitjsefnie wants to merge 6 commits into
Conversation
ADR 0013 designates this module the resolver from a bill slug plus a per-bill ordinal to the readable version file, but it held only the two string helpers. Add the two filesystem functions AgoraDMV#152 needs: `local_versions` for the bill's ordinal/label listing, and `resolve_version_file` for one ordinal's path. Discovery mirrors `scripts/serve_compare.py:_pick_versions` -- same glob, same `(ordinal, stem)` sort key -- so a listing here orders identically to the tool a reader may already have open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`compare` now dispatches on the positional COUNT: two file paths stay the legacy invocation, unchanged; three are a bill slug and two per-bill ordinals resolved under `--bills-dir`; one is a bare slug that lists that bill's local versions and exits 0. Dispatching on count rather than sniffing for `.xml` or a slug regex is what guarantees an existing two-path command can never be re-read as something else. An out-of-range or non-numeric ordinal fails with the same listing as its error message -- the ordinals are per-bill (ADR 0013), so "no version 9" only helps once you can see which versions there are. `--bills-dir` defaults to `bills`, matching `fetch_bills download --output-dir`, which is the command that puts the files there. That literal trips `test_no_source_names_the_download_tree`, so diff_bill.py joins that gate's `_DOWNLOAD_TIER_FILES` with a comment saying why: it addresses the download tier by design, for the same reason the fetchers do. Closes AgoraDMV#152 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The whole point of `compare <slug> <n_old> <n_new>` is discoverability, so it needs a row in the command reference and an example beside the path form. The docs gate only requires one row per subcommand and `compare` is not a new subcommand, so neither is gate-forced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`nargs="*"` was the wrong mechanism for count dispatch. argparse matches
positionals greedily within each run *between* optionals, so the variadic
positional swallowed the whole first run and every invocation with a flag
BETWEEN the two paths regressed from working to exit 2:
compare a.xml --financial b.xml
compare a.xml -o out.json b.xml
compare a.xml --include-unchanged b.xml
compare a.xml --filter SEC b.xml
compare a.xml --format json b.xml
Flags-leading and flags-trailing kept working, which is why an ordering-blind
characterization suite did not see it. Dispatch on positional count still
stands; the collection now goes through argparse's own `parse_intermixed_args`,
switched on at the subparser because it refuses a parser holding subparsers.
A mistyped flag is still a usage error rather than a filename.
Two related fixes:
- A bare slug with no local versions now exits non-zero instead of printing an
empty listing at 0. `compare "$OLD" "$NEW"` with an unset variable collapses
to one argument, which the two-positional parser rejected; a wrapper reading
the exit status has to keep seeing that.
- `isdecimal`, not `isdigit`, for the ordinal: `"³".isdigit()` is True while
`int("³")` raises, so `compare <slug> 1 ³` raised ValueError instead of
printing the version listing. Same fix in version_stems, which reads whatever
filenames are on disk and so could raise from `local_versions`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_DOWNLOAD_TIER_FILES` is consulted by three rules, so putting diff_bill.py in
it switched off all three -- including find_stale_fixture_paths, the rule that
catches a source file reaching for a committed fixture through bills/. That is
the rule this file should keep enforcing hardest on a product module, which has
no business naming an individual bill at all.
Only find_download_tree_names actually fires, and only on one line
(`default=Path("bills")`), so the exemption moves to that rule alone -- the same
shape tests/corpus_paths.py already had there, now named. The set's leading
comment goes back to describing only its own members.
Pinned both ways: the new default is proven exempt, and a bill-level bills/
reference from the same file is proven still caught.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The _IntermixedSubParser guard defended against re-entry that never happens: parse_known_intermixed_args calls the private _parse_known_args2 / _parse_known_args, never the public parse_known_args (verified with inspect.getsource on CPython 3.12 and 3.13). It was dead code defended by a false docstring; remove it and document the verified call path instead. compare with 0 or 4+ positionals exited 1 via SystemExit(message), where argparse's two-positional parser had exited 2. Print the usage message to stderr and exit 2, keeping the usage-error contract a wrapper keys on. Co-Authored-By: Kimi K3 <noreply@kimi.com>
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.
Related issue
Closes #152
What does this change?
comparenow takes<slug> <n_old> <n_new>in addition to two paths, resolving the versions under--bills-dir, and a bare slug prints that bill's local versions instead of an argparse error.Dispatch is on positional count alone, as the issue specifies — no
.xmlsniffing, no slug regex:<slug> <n_old> <n_new>, resolved under--bills-dirNew:
local_versionsandresolve_version_fileinversion_stems.py, the module ADR 0013designates as the resolver. The folder-dive glob and the sort key are lifted verbatim from
scripts/serve_compare.py::_pick_versionsrather than shared into a helper.One thing worth your eye. Count-dispatch needs one variadic positional, and argparse matches
positionals greedily within each run between optionals — so a
nargs="*"positional swallows thefirst run and a second path landing after an intervening flag has no action left to fill. That broke
compare a.xml --financial b.xml, which works today. The fix routes dispatch through argparse's ownparse_intermixed_args, which exists for exactly this, but refuses any parser holding subparsers —so it goes on the subparser via
parser_class. That is the one non-obvious thing in the diff;if you would rather have a bespoke positional collector, say so and I will swap it, though it is more
code and gives worse diagnostics on a mistyped flag.
How to test
Every legacy ordering, run end to end through the real CLI against a corpus pair, not just the parser
object:
A mistyped flag is still a usage error rather than being swallowed as a target:
compare a.xml --fromat json b.xmlexits 2. Arity errors exit 2 with the message on stderr andstdout empty.
TestCompareLegacyTwoPathFormis a characterization class: its assertions are pasted literals withno constant shared with production code, and it passes both before and after — that is what it is
for, and it is not offered as red-green evidence. What it does pin is the ordering dimension, which
is precisely where this change first broke: all six of its ordering cases fail against the earlier
draft of this branch.
The new-behaviour tests do bite: reverting the two production files to
developwhile keeping thetests gives 12 failures in
test_diff_bill.py, andtest_version_stems.pyerrors at collectionon
ImportError, which also takes down 8 pre-existing tests in that module — 22 that never run.Calling that "failures" would overstate it.
Gates:
uv run ruff check .clean ·uv run ruff format --check .178 files · fast1444 passed, 15 skipped, 15 xfailedagainst a1403/15/15baseline · browser6 passed· slow554 passed, 7 skipped, unchanged. Skips unchanged, so nothing needed an allowlist entry. Those arethis machine's numbers, higher than yours — offered as a delta.
A warning-count delta is deliberately not quoted:
pytest-randomlyreorders collection and apre-existing Starlette deprecation aggregates differently per run, so identical trees gave 3, 4 and 5.
Checklist
Closes #...)AI assistance
Generated by Claude Opus 5 (1M context) (brief, implementation, review), Kimi K3 (implementation, verification)
Follow-ups / Known Limitations
--bills-dir'sdefault=Path("bills")tripstest_no_source_names_the_download_tree. Rather than adding the fileto
_DOWNLOAD_TIER_FILES— which is consulted by three rules and would have switched off thecommitted-fixture rule for it too — it goes in a new narrower set consulted by that one rule,
alongside
tests/corpus_paths.py, which already used that carve-out. Proven both ways: the defaultis exempt, and a bill-level fixture reference from the same file is still caught.
--helpno longer namesold_xml/new_xml; all three forms are spelled out in the help textinstead. Unavoidable with one variadic positional.
args.old_xml/args.new_xmlare gone from the parsed namespace. No in-repo caller reads them —scripts/ugly_money_table.pyandscripts/compare_differs.pyshell out with flags trailing — sono compatibility shim was invented.
3_a.xmland3_b.xml) resolve to the first in sorted order. Deterministic,unspecified by the issue, and not a shape the fetchers produce.
diff_pdfgaining the same form, a:versionsingle-token form,repathing ADR 0013's
shared/version_stems.pyreferences, and resolving a bare slug againsttests/corpus/.