Close the v1 follow-ups: #31 decided (no fourth channel), #32 fixed - #35
Merged
Merged
Conversation
#31 -- dispatch(MAPPING) has no channel for a group's group_kwargs. DECIDED, not built: cw grows no fourth channel. ADR-0008 records why, and the evidence it turns on is the live case rather than taste. t/xa is the one fleet repo that passes group_kwargs; its migration landed on the two-call form and reported preferring it -- splitting the build from the run produced a pure mk_parser() that a test can inspect, which is what made 14 grammar tests possible in a repo that had had zero. The thing xa actually passed, group_kwargs={'help': ...}, turns out to be INERT (the parent's listing row is fed by title), so the motivating case for a new keyword was a case that needed a better error message. Each of the three options the issue weighed damages something cw relies on: a (funcs, kwargs) mapping value adds a seventh form of obj and breaks the rule that a value's KIND decides its meaning; group_config= is a fourth behaviour-carrying keyword that is not a seam, needing an ADR-0001 amendment for something with no default to name; a reserved key in config puts add_subparsers keywords into a mapping documented as per-PARAMETER particulars. What cw owed a caller instead was that every rejected spelling name the accepted one. All three were already errors; none said what works: - mk_parser/dispatch(..., group_kwargs=/group_name=/namespace=/namespace_kwargs=) join cw.cli.SEAMS_ELSEWHERE, which exists for exactly this. They previously reported that argparse.ArgumentParser has no such keyword and listed argparse's parameters -- true, and useless. - {'archive': (ARCHIVE, {'title': ...})} raises CommandTreeError naming the pair. It previously died inside command_name saying a dict has no __name__. - config={'archive': {'title': ...}} appends the recipe, but only when EVERY unknown key names an add_subparsers keyword, so a misspelt command still gets the short error. All three point at one string, cw.cli.TWO_CALL_GROUP_RECIPE, so the advice cannot drift. It names the two things that bite people here, both learned from real migrations: title-not-help, and that cw.run RETURNS the exit code where argh's parser.dispatch() raised it -- the step two independent waves called the highest-risk part of a modern-API migration, and which nothing catches (unit tests pass, a --help diff shows nothing). README: the add_commands section now shows the MAPPING form with group_kwargs. The pilot had to read cw/cli.py to confirm a mapping was accepted there at all -- the docstring said so, the README only ever showed a list. #32 -- house-rule naming. - grammar.infer_specs -> _infer_specs (also out of __all__) and grammar.finalise_spec -> _finalise_spec. Both are module-local in practice; verified fleet-wide that nothing outside cw/grammar.py names either. - testing.pinned_env is now a @contextlib.contextmanager function rather than a lowercase-named class. Same name, same `with pinned_env():` call shape, so nothing breaks; restoration moves into a finally, so an exception inside the block no longer leaks the pins. contextlib joins the D4 allowed-import set -- stdlib, and the set is asserted by tests/test_testing.py, which is updated. - cw/resolution.py: 70% -> 100%, by TESTS, not by refactoring. The issue is explicit that this is pre-existing code with a live dependent (t/theremin). Almost every uncovered line was an error path, which is the shape of the risk: a resolver whose happy path one dependent exercises and whose refusals nobody does is a module where a rewrite can silently start accepting garbage. 53 tests characterize what it does today, error messages included, including resolve_object, which had none at all and whose own TODO asks for a merge. One quirk characterized rather than changed: parse_ast_spec('len(((') is routed to the dot-path parser by the paren pre-check and refused there. cw/util.py: already deleted in the v1 land (54369a0). Nothing to do; ADR-0008 says so, so the next reader does not go looking. Verified: 932 passed, 2 skipped (was 878/2), also green under CI's own doctest flags. Parity gate: 8 shapes / 137 cases identical, exit 0. cw/cli.py stays at 100%; the four remaining misses in the package are pre-existing and unmoved. Four mutations -- removing each new diagnostic, and stopping pinned_env from restoring -- each turn exactly the matching tests red, and both new README examples were proved to be executed by breaking them. Closes #31, closes #32. Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr
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.
Closes #31, closes #32.
Two follow-ups cut from the v1 land, plus the
cw/util.pyquestion the foundation phase left open.#31 —
dispatch(MAPPING)and a group'sgroup_kwargsDecided, not built. cw grows no fourth channel. ADR-0008 records why, and the evidence it turns on is the live case rather than taste.
t/xais the one fleet repo that passesgroup_kwargs. Its migration off argh landed while this issue was open, on the two-call form, and the migrator's report was unambiguous:Three things follow from that, all checkable in the repo today:
mk_parser()a test can inspect.t/xahad no CLI-shape test before — nothing assertedgen-secretwas spelled that way, or thatarchive listdid not shadow the top-levellist, though thirteen__name__mutations depended on it. It has fourteen now, and they exist becausemk_parserbecame a function returning a parser.cw.mk_parser(...)already carriesArghHelpFormatter.group_kwargs={'help': ...}, which is inert — the parent's listing row is fed bytitle(archivegroup has no help text inxa --help— group_kwargs useshelp, argparse readstitlethorwhalen/xa#13). So the motivating case for a new keyword was, on inspection, a case that needed a better error message.Each option the issue weighed damages something cw relies on elsewhere:
(funcs, kwargs)mapping valueobjand breaks the rule the module is built on — a value's kind decides its meaning. A tuple is already an iterable, i.e. already a group.group_config=onmk_parser/dispatchadd_commandsalready does.configconfigis per-parameter particulars. A group'sadd_subparserskeywords are particulars of no parameter, and the key would have to be a sentinel to avoid colliding with a command named__group__.What cw did owe a caller: every rejected spelling must name the accepted one. All three were already errors; none of them said what works. That is the actual defect, and it is what this PR fixes:
mk_parser/dispatch(..., group_kwargs=)— plusgroup_name, and argh's pre-0.30namespace/namespace_kwargs— joincw.cli.SEAMS_ELSEWHERE, which already existed for exactly this ("a real cw keyword, but not on this call"). They previously reported thatargparse.ArgumentParserhas no such keyword and listed argparse's parameters: true, and useless.{'archive': (ARCHIVE, {'title': ...})}now raisesCommandTreeErrornaming the pair. It previously died insidecommand_namecomplaining adicthas no__name__.config={'archive': {'title': ...}}appends the recipe — but only when every unknown key names anadd_subparserskeyword, so an ordinary misspelt command still gets the short, relevant error.All three point at one string,
cw.cli.TWO_CALL_GROUP_RECIPE, so there is no second copy of the advice to drift. It names the two things that bite people on this route, both learned from real migrations rather than guessed:titleneverhelp, and thatcw.runreturns the exit code where argh'sparser.dispatch()raised it — the step two independent migration waves called the highest-risk part of a modern-API migration, and which nothing catches (unit tests pass; a--helpdiff shows nothing).Docs: the README's
add_commandssection now shows the mapping form withgroup_kwargs. The pilot had to readcw/cli.pyto confirm a mapping was accepted there at all — the docstring said so, the README only ever showed a list.#32 — house-rule naming, and resolution.py's uncovered third
grammar.infer_specs→_infer_specs(also out of__all__),grammar.finalise_spec→_finalise_spec. Both module-local in practice; verified fleet-wide that nothing outsidecw/grammar.pynames either.testing.pinned_envis now a@contextlib.contextmanagerfunction rather than a lowercase-named class. Same name, samewith pinned_env():call shape, so nothing breaks — and restoration moves into afinally, so an exception inside the block no longer leaks the pins.contextlibjoins the D4 allowed-import set (stdlib; the set is asserted by a test, which is updated, and the copy-it-into-a-repo test still passes).cw/resolution.py: 70% → 100%, by tests, not by refactoring, as the issue asked. Almost every uncovered line was an error path — which is the shape of the risk: a resolver whose happy path one dependent exercises and whose refusals nobody does is a module where a rewrite can silently start accepting garbage. 53 tests characterize what it does today, error messages included, includingresolve_object, which had no test at all and whose own TODO asks for a merge. One quirk characterized rather than changed:parse_ast_spec('len(((')is routed to the dot-path parser by the paren pre-check and refused there.cw/util.pyAlready deleted in the v1 land (
54369a0). Nothing to do. ADR-0008 says so explicitly, so the next reader does not go looking for it.Verification
-o doctest_optionflags='ELLIPSIS IGNORE_EXCEPTION_DETAIL'), which drop theNORMALIZE_WHITESPACEpyprojectsets.8 shapes / 137 cases: identical, exit 0. No grammar change, no--helpchange, andcw/tests/goldens/is untouched — the review gate ADR-0005 rule 2 defines.cw/cli.pystays at 100% statement coverage; the four remaining misses in the package are pre-existing and unmoved (verified bygit stash).SEAMS_ELSEWHERE(3 red), removing the(funcs, kwargs)recognition (1 red), removing the config hint (1 red), and stoppingpinned_envrestoring (2 red).test_docs_examplesfail.ruff checkandruff format --checkclean.Release
Patch. No grammar change and no golden diff, so ADR-0005 rule 2 does not apply. Patch keeps it inside the
cw>=0.1.1,<0.2pin the migrated repos carry, which matters — the improved error messages are for repos that are mid-migration right now.