diff --git a/README.md b/README.md index 05abaaa..9889fd9 100644 --- a/README.md +++ b/README.md @@ -292,9 +292,69 @@ what argh does — but then the *root* parser's own `--help` uses argparse's for exactly as under argh. Pass `formatter_class=cw.ArghHelpFormatter` yourself if you want the root to match too. -`dispatch({'archive': {...}})` has no channel for `group_kwargs`; a group declared that way -gets an empty listing row where argh printed its `title`. Use `mk_parser` + `add_commands` + -`run` when you need one ([#31](https://github.com/i2mint/cw/issues/31)). +**`add_commands` takes any `obj` `dispatch` takes, a mapping included** — so a group whose +members you want to name yourself is a mapping, exactly as it would be inside `dispatch`: + +```python +>>> parser = cw.mk_parser([], prog='priv') +>>> _ = cw.add_commands(parser, {'st': status}, group_name='git_ops', +... group_kwargs={'title': 'Git operations'}) +>>> cw.run(parser, ['git_ops', 'st']) +0 +``` + +### Wanting a group `title` from the mapping form + +`cw.dispatch({'archive': {...}})` builds the group, but a single mapping has nowhere to put +the group's own `add_subparsers` keywords. **That is deliberate, not a gap**: `add_commands` +already takes a mapping *and* `group_kwargs`, so the answer is two calls rather than a +fourth behaviour-carrying keyword on `dispatch` +([#31](https://github.com/i2mint/cw/issues/31), +[ADR-0008](docs/adr/0008-no-fourth-channel-for-group-kwargs.md)). The console-script idiom +is: + +```python +TOP_COMMANDS = {"list": list_cmd, "info": info_cmd} +ARCHIVE_COMMANDS = {"list": archive_list_cmd, "log": archive_log_cmd} + + +def mk_parser(): + parser = cw.mk_parser(TOP_COMMANDS, prog="xa") + cw.add_commands( + parser, + ARCHIVE_COMMANDS, + group_name="archive", + group_kwargs={"title": "Postmortem archive"}, + ) + return parser + + +def main(): + raise SystemExit(cw.run(mk_parser())) +``` + +Two things bite people here, and cw's error messages now name both: + +- **`group_kwargs['help']` is silently inert; you want `'title'`.** The group's row in the + *parent's* `--help` is fed by `add_parser(help=...)`, which both argh and cw source from + `group_kwargs['title']`. `help` is forwarded to `add_subparsers()`, where argparse + accepts it and renders it nowhere a reader looks. cw reproduces argh exactly — that is + the product — but it is a trap that shipped a group description nobody ever saw in at + least one fleet repo. +- **`cw.run` *returns* an exit code where argh's `parser.dispatch()` raised it.** Splitting + the build from the run is precisely when the `raise SystemExit(...)` gets dropped, and a + console script that starts exiting `0` on a usage error breaks every CI step that checks + `$?`. Nothing else catches it: unit tests pass, and a `--help` diff shows nothing. + +Trying it any of the other three ways is an error that names this one: + +```pycon +>>> cw.dispatch({"archive": {"log": status}}, [], group_kwargs={"title": "T"}) +Traceback (most recent call last): + ... +TypeError: 'group_kwargs' cannot be passed here. cw's group keywords belong to +cw.add_commands, ... +``` **A mapping value must be the commands, not the factory that returns them.** A callable value is always a *command*, because there is no way to tell a zero-argument factory from a @@ -478,7 +538,7 @@ Python 3.10+. ## Design notes -Six decisions, with their evidence, in [`docs/adr/`](docs/adr/): +Eight decisions, with their evidence, in [`docs/adr/`](docs/adr/): | | | |---|---| @@ -488,6 +548,8 @@ Six decisions, with their evidence, in [`docs/adr/`](docs/adr/): | [ADR-0004](docs/adr/0004-grammar-errata.md) | `group_kwargs`, mapping-key naming, MODERN's help column | | [ADR-0005](docs/adr/0005-release-and-rollback-policy.md) | Release, pinning and rollback — `cw.ARGH` is frozen once published | | [ADR-0006](docs/adr/0006-the-v1-cut-list.md) | What v1 does not ship, and where each cut comes back | +| [ADR-0007](docs/adr/0007-what-the-adversarial-review-changed.md) | What three adversarial reviews changed, and the blind spots that hid it | +| [ADR-0008](docs/adr/0008-no-fourth-channel-for-group-kwargs.md) | Why a group's `group_kwargs` gets no channel in the mapping form | Two properties worth stating because they are easy to lose and hard to get back: `cw.mk_parser` returns a **plain** `ArgumentParser`, and **`import cw` pulls stdlib only** — diff --git a/cw/cli.py b/cw/cli.py index 89af9fb..5380ffe 100644 --- a/cw/cli.py +++ b/cw/cli.py @@ -83,13 +83,40 @@ class BoundKeywordWarning(UserWarning): #: reports when it catches one. USAGE_ERROR_CODE = 2 +#: The answer to "a group in the mapping form wants ``title=``", which is issue #31 and +#: which cw deliberately does **not** give a fourth behaviour-carrying keyword. A mapping +#: value already *is* a group; what it has nowhere to put is the group's own +#: ``add_subparsers`` keywords. Those belong to :func:`add_commands`, which takes a mapping +#: too -- so the answer is to build and add in two calls rather than one. See ADR-0008. +TWO_CALL_GROUP_RECIPE = ( + "cw's group keywords belong to cw.add_commands, not to cw.mk_parser or cw.dispatch. " + "A mapping VALUE is already a group -- cw.dispatch({'archive': {'ls': ls}}) -- but a " + "single mapping has nowhere to put the group's own add_subparsers keywords, so build " + "the parser and add the group as two calls:\n" + " parser = cw.mk_parser(TOP_COMMANDS, prog='xa')\n" + " cw.add_commands(parser, ARCHIVE_COMMANDS, group_name='archive',\n" + " group_kwargs={'title': 'Postmortem archive'})\n" + " raise SystemExit(cw.run(parser))\n" + "Note the two details that catch everyone: the group's row in the PARENT's --help is " + "fed by group_kwargs['title'], never ['help'] (which argparse accepts and renders " + "nowhere); and cw.run RETURNS an exit code where argh's parser.dispatch() raised it, " + "so it must be wrapped in `raise SystemExit(...)` or every usage error exits 0." +) + #: cw keywords that are real, but not on *this* call. Without this, ``mk_parser(f, #: egress=...)`` reports that ``argparse.ArgumentParser`` has no such keyword and lists #: argparse's parameters -- true, and the least useful true thing to say. A seam is one #: keyword argument, but not every seam is on every entry point: ``egress`` runs after the #: call, so it belongs to :func:`run` and :func:`dispatch`, and ``decode`` shapes the #: parser, so it belongs to :func:`mk_parser`, :func:`dispatch` and :func:`add_commands`. +#: The ``group_*`` entries are not seams at all -- they are :func:`add_commands` keywords +#: that a caller reasonably tries on the mapping form first (issue #31). SEAMS_ELSEWHERE = { + "group_name": TWO_CALL_GROUP_RECIPE, + "group_kwargs": TWO_CALL_GROUP_RECIPE, + # argh's pre-0.30 spellings, which cw.add_commands still accepts. + "namespace": TWO_CALL_GROUP_RECIPE, + "namespace_kwargs": TWO_CALL_GROUP_RECIPE, "egress": ( "cw's egress seam turns a return value into output, which happens when a command " "runs -- so it is a keyword of cw.run and cw.dispatch, not of cw.mk_parser. To " @@ -140,8 +167,18 @@ def _new_parser(convention: Convention, parser_kwargs: dict) -> argparse.Argumen parser_kwargs.setdefault("formatter_class", ArghHelpFormatter) misplaced = sorted(set(parser_kwargs) & set(SEAMS_ELSEWHERE)) if misplaced: + # Several keywords may share one explanation -- group_name and group_kwargs both + # point at TWO_CALL_GROUP_RECIPE -- and printing that paragraph twice is worse + # than printing it once. Name every misplaced keyword, say each thing once. + seen, reasons = set(), [] + for name in misplaced: + reason = SEAMS_ELSEWHERE[name] + if reason not in seen: + seen.add(reason) + reasons.append(reason) raise TypeError( - "; ".join(f"{name}: {SEAMS_ELSEWHERE[name]}" for name in misplaced) + f"{', '.join(repr(name) for name in misplaced)} cannot be passed here. " + + " ".join(reasons) ) try: return argparse.ArgumentParser(**parser_kwargs) @@ -407,6 +444,24 @@ def _add_group( ) +#: ``add_subparsers`` keywords, which are what somebody is reaching for when a ``config`` +#: key at the group level names one of these rather than a command (issue #31's option 1). +#: Used only to add a sentence to an error that was already going to be raised. +GROUP_KWARG_NAMES = frozenset( + { + "title", + "description", + "prog", + "parser_class", + "action", + "dest", + "required", + "help", + "metavar", + } +) + + def _check_config_keys(tree: Mapping, config: Mapping, *, what: str) -> None: """A ``config`` key naming no command is a startup error, never a silent no-op. @@ -414,18 +469,26 @@ def _check_config_keys(tree: Mapping, config: Mapping, *, what: str) -> None: (``hyphenate_groups``) and every config entry keyed by the old name quietly stops applying. Keys and names go through one naming function, so a mismatch is a bug, and a bug should be loud. + + A key that names an ``add_subparsers`` keyword instead gets the extra sentence, because + ``config={'archive': {'title': ...}}`` is the other thing a reader tries when a group in + the mapping form wants a title, and "matches no command" is a true answer to a question + they did not ask. """ unknown = [key for key in config if key not in tree] if unknown: known = ", ".join(tree) or "(none)" plural = len(unknown) > 1 - raise GrammarError( + message = ( f"config key{'s' if plural else ''} " f"{', '.join(repr(key) for key in unknown)} " f"{'match' if plural else 'matches'} no {what}. " f"Known {what} names: {known}. Note that names are hyphenated by the " "convention, so a config must be keyed the way the command line is typed." ) + if GROUP_KWARG_NAMES.issuperset(unknown): + message += " " + TWO_CALL_GROUP_RECIPE + raise GrammarError(message) def _add_tree( @@ -549,6 +612,24 @@ def add_commands( >>> _ = add_commands(parser, [status], group_name='git_ops') >>> parser.format_usage() 'usage: priv [-h] {git_ops} ...\\n' + + ``obj`` is anything :func:`cw.dispatch` takes, **a mapping included** -- which is what + makes this the answer to "a group in the mapping form wants ``title=``" (issue #31, + ADR-0008). Seed the parser with :func:`mk_parser` rather than ``ArgumentParser()`` so + the root gets cw's formatter too, and remember that ``cw.run`` *returns* the exit code + rather than raising it: + + >>> parser = cw.mk_parser({'info': status}, prog='xa') + >>> _ = add_commands(parser, {'log': status}, group_name='archive', + ... group_kwargs={'title': 'Postmortem archive'}) + >>> parser.format_usage() + 'usage: xa [-h] {info,archive} ...\\n' + >>> 'Postmortem archive' in parser.format_help() + True + + The group's row in the **parent's** ``--help`` is fed by ``group_kwargs['title']``, and + only by that. ``help`` is accepted, forwarded to ``add_subparsers`` and rendered + nowhere -- see :func:`_add_group`, which is where that is implemented and explained. """ convention = _with_decode(convention, decode) group_name = group_name if group_name is not None else namespace diff --git a/cw/commands.py b/cw/commands.py index 7c0e039..3b692ce 100644 --- a/cw/commands.py +++ b/cw/commands.py @@ -64,6 +64,18 @@ class CommandTreeError(TypeError): """``obj`` does not describe a command tree, and guessing would ship a wrong CLI.""" +def _two_call_group_recipe() -> str: + """:data:`cw.cli.TWO_CALL_GROUP_RECIPE`, imported late. + + ``cw.cli`` imports this module at module scope, so the text cannot be imported the + other way round at module scope. It is one string in one place either way -- there is + no second copy of the advice to drift. + """ + from cw.cli import TWO_CALL_GROUP_RECIPE + + return TWO_CALL_GROUP_RECIPE + + def import_object(ref: str) -> Any: """``'pkg.mod:name'`` -> the object, imported now. @@ -245,12 +257,44 @@ def commands_from(obj: Any, /, *, convention=None, _depth: int = 0) -> CommandTr ) +def _looks_like_a_funcs_kwargs_pair(value: Any) -> bool: + """Is this the ``(funcs, group_kwargs)`` pair that issue #31 weighed and rejected? + + A reader who wants a group ``title=`` out of the mapping form reaches for + ``{'archive': (ARCHIVE_COMMANDS, {'title': ...})}`` before anything else. cw does not + accept that spelling -- see ADR-0008 -- and without this check the attempt dies inside + :func:`command_name` complaining that a ``dict`` has no ``__name__``, which names + neither what was tried nor what works. + + The shape is narrow on purpose, so no legitimate group is mistaken for it: a two-member + sequence whose **first** member is not itself a command and whose **second** member is a + mapping. ``{'grp': [f, g]}`` has a callable first member; ``{'grp': [f, {'a': g}]}`` has + a callable first member too. Both fall through to the ordinary path. + + >>> def f(): ... + >>> _looks_like_a_funcs_kwargs_pair(([f], {'title': 'T'})) + True + >>> _looks_like_a_funcs_kwargs_pair([f, f]), _looks_like_a_funcs_kwargs_pair({'a': f}) + (False, False) + """ + if not isinstance(value, (tuple, list)) or len(value) != 2: + return False + funcs, kwargs = value + return isinstance(kwargs, Mapping) and not is_command(funcs) + + def _from_mapping(obj: Mapping, *, convention, _depth: int) -> CommandTree: """Each key names its value: a command if the value is callable, a group otherwise.""" tree: CommandTree = {} for key, value in obj.items(): if isinstance(value, str): value = import_object(value) + if _looks_like_a_funcs_kwargs_pair(value): + raise CommandTreeError( + f"the value of {key!r} looks like a (commands, group_kwargs) pair, and cw " + "does not accept one: a mapping value is a group, and nothing more. " + + _two_call_group_recipe() + ) if is_command(value): _put(tree, _named(key, convention=convention), value) elif _depth >= MAX_GROUP_DEPTH: diff --git a/cw/grammar.py b/cw/grammar.py index 86f82b6..f56b529 100644 --- a/cw/grammar.py +++ b/cw/grammar.py @@ -55,7 +55,6 @@ "modern_decode", "cli_name", "command_name", - "infer_specs", "specs_for_function", "BY_NAME_IF_HAS_DEFAULT", "BY_NAME_IF_KWONLY", @@ -537,7 +536,7 @@ def _hints_of(func: Any, *, resolve: bool) -> Dict[str, Any]: return raw -def infer_specs( +def _infer_specs( func: Any, /, *, @@ -555,7 +554,7 @@ def infer_specs( >>> def f(path, *, verbose: bool = False, tags: list = None): ... ... - >>> for spec in infer_specs(f): + >>> for spec in _infer_specs(f): ... print(spec.param_name, spec.flags, spec.extra) path ['path'] {} verbose ['-v', '--verbose'] {} @@ -667,7 +666,7 @@ def _guess_from_default(spec: ArgSpec) -> Dict[str, Any]: return guessed -def finalise_spec( +def _finalise_spec( spec: ArgSpec, /, *, parser_adds_help: bool = True, default_in_help: bool = True ) -> ArgSpec: """The last three things argh does to every spec, in argh's order. @@ -680,7 +679,7 @@ def finalise_spec( 3. ``-h`` is taken away from whoever inferred or declared it, because ``--help`` owns it. A parameter named ``host`` never gets a short flag. - >>> finalise_spec(ArgSpec('host', ['-h', '--host'], default='localhost')).flags + >>> _finalise_spec(ArgSpec('host', ['-h', '--host'], default='localhost')).flags ['--host'] """ spec.extra.update(_guess_from_default(spec)) @@ -747,7 +746,9 @@ def specs_for_function( config = dict(config or {}) use_hints = convention.hints_when_declared or not (declared or config) - specs = infer_specs(func, convention=convention, decode=decode, use_hints=use_hints) + specs = _infer_specs( + func, convention=convention, decode=decode, use_hints=use_hints + ) by_param = {spec.param_name: spec for spec in specs} order = list(by_param) @@ -772,7 +773,7 @@ def specs_for_function( raise _no_such_parameter(func, key, by_param) return [ - finalise_spec( + _finalise_spec( by_param[name], parser_adds_help=parser_adds_help, default_in_help=convention.default_in_help, diff --git a/cw/testing.py b/cw/testing.py index 2e6f9e3..f48126e 100644 --- a/cw/testing.py +++ b/cw/testing.py @@ -1,8 +1,8 @@ """Record a CLI's behaviour before a migration and assert it after. -This file is **standalone** (D4). Its module-level imports are ``argparse``, ``difflib``, -``json``, ``os``, ``re``, ``shlex``, ``subprocess`` and ``sys`` -- stdlib, all of it, and no -``cw`` anywhere. That is not tidiness; it is the whole point. The fleet has 22 repos whose +This file is **standalone** (D4). Its module-level imports are ``argparse``, +``contextlib``, ``difflib``, ``json``, ``os``, ``re``, ``shlex``, ``subprocess`` and +``sys`` -- stdlib, all of it, and no ``cw`` anywhere. That is not tidiness; it is the whole point. The fleet has 22 repos whose CLI is being deleted and 35 that are argparse-only and will never depend on ``cw``, and all of them want the same thing: *proof that the command line did not change*. Copy this one file into such a repo and it works. @@ -72,6 +72,7 @@ """ import argparse +import contextlib import difflib import json import os @@ -403,37 +404,55 @@ def _env_for(env=None) -> dict: return resolved -class pinned_env: +@contextlib.contextmanager +def pinned_env(env=None): """Context manager applying :data:`RECORDING_ENV` to the **current** process. :func:`characterize` hands the pins to a subprocess, where they belong. :func:`parity` runs in-process and needs the same pins applied here instead -- ``COLUMNS`` above all, since that is what ``argparse`` wraps to. + Args: + env: Extra pins, applied over :data:`RECORDING_ENV`. + + Yields: + The mapping that was applied, which is what ``as`` gives you. + >>> with pinned_env(): ... os.environ['COLUMNS'] '100' + >>> with pinned_env({'COLUMNS': '80'}) as pins: + ... (os.environ['COLUMNS'], pins['COLUMNS']) + ('80', '80') + + The name is lowercase because it reads as a statement rather than as a type, and it is + a function rather than a class because there is no object here worth having -- the + house rule is functional over OOP, and ``contextlib`` is where the state machine goes. + Restoration is in a ``finally``, so an exception inside the block does not leak the + pins into the rest of the process: + + >>> before = os.environ.get('COLUMNS') + >>> try: + ... with pinned_env(): + ... raise RuntimeError('boom') + ... except RuntimeError: + ... pass + >>> os.environ.get('COLUMNS') == before + True """ - - def __init__(self, env=None): - self.env = dict(RECORDING_ENV, **(env or {})) - self._saved = {} - - def __enter__(self): - for name in list(self.env) + list(UNSET_ENV): - self._saved[name] = os.environ.get(name) - os.environ.update(self.env) - for name in UNSET_ENV: - os.environ.pop(name, None) - return self - - def __exit__(self, *exc_info): - for name, value in self._saved.items(): + env = dict(RECORDING_ENV, **(env or {})) + saved = {name: os.environ.get(name) for name in list(env) + list(UNSET_ENV)} + os.environ.update(env) + for name in UNSET_ENV: + os.environ.pop(name, None) + try: + yield env + finally: + for name, value in saved.items(): if value is None: os.environ.pop(name, None) else: os.environ[name] = value - return False def _exit_status(exc: SystemExit) -> tuple: diff --git a/docs/adr/0008-no-fourth-channel-for-group-kwargs.md b/docs/adr/0008-no-fourth-channel-for-group-kwargs.md new file mode 100644 index 0000000..db1a070 --- /dev/null +++ b/docs/adr/0008-no-fourth-channel-for-group-kwargs.md @@ -0,0 +1,129 @@ +# ADR-0008: No fourth channel for a group's `group_kwargs` + +- **Status:** accepted +- **Date:** 2026-08-30 +- **Deciders:** Thor Whalen +- **Issue:** [#31](https://github.com/i2mint/cw/issues/31) + +## Context + +`cw.dispatch({'archive': {'ls': ls}})` builds the group. It has nowhere to say +`title='Postmortem archive'`, so the group's row in the parent's `--help` is bare where argh +printed a title. `cw.add_commands(parser, obj, group_name=..., group_kwargs=...)` **does** +have that channel and is byte-identical to argh at both levels — but reaching it means +splitting one `dispatch` call into `mk_parser` + `add_commands` + `run`. + +Issue #31 weighed three ways to close the gap in the mapping form itself: + +1. a mapping **value** that is a `(funcs, kwargs)` pair — `{'archive': (ARCHIVE, {'title': ...})}`; +2. a **`group_config=`** mapping on `mk_parser` / `dispatch`, keyed by group name; +3. a **reserved key** inside the existing `config` tree — `config['archive']['__group__']`. + +The issue recorded that none was obviously right, which is why it was cut from v1. + +**Then the live case shipped.** `t/xa` is the one fleet repo that passes `group_kwargs`, and +its migration off argh landed while this was still open ([xa#14](https://github.com/thorwhalen/xa/pull/14)). +It used the two-call form. The migrator's report is the evidence this ADR turns on, and it +is worth quoting rather than paraphrasing: + +> Real but small, and the workaround is good — the gap is documentation, not capability. +> […] Once found, `cw.mk_parser(TOP, prog='xa')` + one `add_commands` reads as the same +> two-call shape argh had, seeds the formatter correctly (avoiding the reflow trap by +> construction), and forced a pure `mk_parser()` — which turned out to be the best thing in +> the xa PR, because it made 14 grammar tests possible. **I would choose it again over a +> hypothetical `dispatch(MAPPING, group_config=...)`.** + +Three findings from that migration, all verifiable in the repo today: + +- The two-call form is not a downgrade. Splitting the build from the run produced a pure + `mk_parser()` that a test can inspect without running anything. `t/xa` had **no** CLI-shape + test before the migration — nothing asserted that `gen-secret` was spelled that way, or + that `archive list` did not shadow the top-level `list`, though thirteen `__name__` + mutations depended on it. It has fourteen now, and they exist because `mk_parser` became a + function that returns a parser. +- The formatter trap is avoided **by construction** on this route. Seeding with + `cw.mk_parser(...)` rather than a bare `argparse.ArgumentParser()` means `formatter_class` + is already `cw.ArghHelpFormatter`, so `add_commands`' `_child_formatter` propagates it and + `--help` does not reflow. A `group_config=` keyword would have kept callers on `dispatch`, + where that is not a hazard either — but it would also have kept them away from the parser + object, which is where the tests live. +- The thing `t/xa` actually passed was `group_kwargs={'help': ...}`, which is **inert**. The + parent's listing row comes from `title`; `help` reaches `add_subparsers()`, where argparse + accepts it and renders it nowhere. That string had never been shown to anyone + ([xa#13](https://github.com/thorwhalen/xa/issues/13)). So the motivating case for a new + channel was, on inspection, a case that needed a **better error message**, not a new + keyword. + +## Decision + +**cw grows no fourth channel. The mapping form stays as it is, `add_commands` remains the +one place a group's `add_subparsers` keywords are passed, and the two-call form is promoted +from a footnote to the documented answer.** + +Each rejected option, and why: + +| option | rejected because | +|---|---| +| `(funcs, kwargs)` pair as a mapping value | It adds a **seventh** form of `obj` and breaks the rule the whole module is built on: *the meaning of a value is decided by the value's kind*. A tuple is already an iterable, i.e. already a group; making a two-member tuple mean something else makes `{'grp': (f, g)}` and `{'grp': ([f], {})}` differ in kind, which nobody can read. | +| `group_config=` on `mk_parser` / `dispatch` | It is a fourth behaviour-carrying keyword, so ADR-0001's seam table needs an amendment for something that is **not a seam** — no default to name, no replacement to point at. It is also a second way to do what `add_commands` already does, and the live case reports preferring the existing way. | +| a reserved key in `config` | `config` is *per-parameter* particulars, keyed `{command: {param: add_argument_kwargs}}`. A group's `add_subparsers` keywords are particulars of no parameter. The reserved key would have to be a sentinel to avoid colliding with a command called `__group__`, which means a new public name in the facade for a keyword one repo passes. | + +**What cw owes a caller instead: 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 ADR's implementation fixes: + +- `mk_parser(..., group_kwargs=...)` / `dispatch(..., group_kwargs=...)` — and `group_name`, + plus argh's pre-0.30 `namespace` / `namespace_kwargs` spellings — join + `cw.cli.SEAMS_ELSEWHERE`, which already exists for exactly this ("a real cw keyword, but + not on *this* call"). 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 by + name. It previously died inside `command_name` complaining that a `dict` has no + `__name__`, which names neither what was tried nor what works. +- `config={'archive': {'title': ...}}` already raised `config key 'title' matches no + command`, which is a true answer to a question the caller did not ask. It now appends the + recipe when **every** unknown key names an `add_subparsers` keyword — 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. The recipe names the two things that bite people on this route, both +learned from real migrations rather than guessed: + +1. `group_kwargs['title']`, never `['help']` — the xa#13 trap above; and +2. `raise SystemExit(cw.run(parser))`, because `cw.run` **returns** an exit code where + argh's `parser.dispatch()` raised it. Splitting a `dispatch` call in two is precisely + when that gets dropped, and a console script that starts exiting `0` on a usage error + breaks every CI step that checks `$?`. Two independent migration waves reported this as + the highest-risk step in a modern-API migration, and nothing catches it: unit tests pass, + and a `--help` diff shows nothing. + +## Consequences + +- **Nothing a user sees moves.** No grammar change, no new keyword, no `--help` difference. + The parity gate stays at `8 shapes / 137 cases: identical`, which it does. +- `t/xa` needs no change. Its `mk_parser()` is now the documented shape rather than a + workaround, and the comment in `xa/cli.py` explaining why it is two calls can stay as it + is — it is correct. +- The README's `add_commands` section now shows the **mapping** form with `group_kwargs`. + The pilot had to read `cw/cli.py:522-579` to confirm a mapping was accepted there at all; + the docstring said so and the README only ever showed a list. +- **If this decision is wrong, the symptom will be specific and countable**: repos that end + up on `mk_parser` + `add_commands` *only* for a group title, and would otherwise have been + one `dispatch` line. There is one such repo today. Revisit at three, and if it is + revisited, `group_config=` is the option to revisit — it is the only one of the three that + does not damage a rule cw relies on elsewhere. +- ADR-0001's seam table is **unamended**, which is the point. The `NOT seams:` block gains + nothing because nothing was added. + +## Also settled here + +Two loose ends from the same review, recorded so they are not rediscovered: + +- **`cw/util.py`** — flagged by the foundation phase as an empty module (docstring only, + zero importers). It was already deleted in the v1 land (commit `54369a0`); nothing to do, + and this line exists so the next reader does not go looking for it. +- **`group_kwargs['help']` stays inert.** Making cw raise or warn on it would be a + divergence from argh in the one direction cw does not go, and it would fire on every + invocation of an already-migrated repo. It is documented in the README and named in the + error message instead. `cw.MODERN` is where a future ADR could make it loud, since MODERN + is allowed to differ; it does not do so in this one. diff --git a/docs/adr/README.md b/docs/adr/README.md index 08ee919..f3a9dbe 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -1,6 +1,6 @@ # Architecture decision records -Seven decisions, written while cw v1 was built, in the Nygard format used across the fleet +Eight decisions, written while cw v1 was built and closed out, in the Nygard format used across the fleet (`docs/adr/NNNN-slug.md`, immutable once accepted; change one by writing a new ADR that supersedes it, never by editing an accepted **Decision** section in place). @@ -13,6 +13,7 @@ supersedes it, never by editing an accepted **Decision** section in place). | [0005](0005-release-and-rollback-policy.md) | Release, pinning and rollback policy for the ~34 repos that will depend on cw | [#12](https://github.com/i2mint/cw/issues/12) | | [0006](0006-the-v1-cut-list.md) | The v1 cut list: what cw deliberately does **not** ship, and where each comes back | [#13](https://github.com/i2mint/cw/issues/13) | | [0007](0007-what-the-adversarial-review-changed.md) | What three adversarial reviews changed: the "invisible" positional divergence, the formatter rule, the gate's blind spot | [#25](https://github.com/i2mint/cw/issues/25) | +| [0008](0008-no-fourth-channel-for-group-kwargs.md) | No fourth channel for a group's `group_kwargs`: the mapping form stays as it is, and every rejected spelling names the accepted one | [#31](https://github.com/i2mint/cw/issues/31) | Read them in order. 0001 is the one that must outlive the session: it is the budget every later addition is spent against. diff --git a/tests/test_cli.py b/tests/test_cli.py index c823883..05548bc 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -664,3 +664,118 @@ def test_ingress_says_there_is_no_such_keyword_anywhere(self): def test_an_ordinary_typo_still_blames_argparse(self): with pytest.raises(TypeError, match="argparse.ArgumentParser accepts"): cw.mk_parser(echo, prgo="x") + + +# ======================================================================================= +# cw#31: a group in the mapping form wants `group_kwargs` +# +# The decision (ADR-0008) is that cw grows no fourth channel for this: `add_commands` +# already takes a mapping AND `group_kwargs`, so the answer is two calls. What cw owes a +# caller is that the three spellings the issue weighed and rejected each say so, rather +# than failing somewhere that names neither what was tried nor what works. +# +# `t/xa` is the live case. Its shape is reproduced at the bottom of this section, because +# a recipe printed in an error message that nobody executes is a recipe that rots. +# ======================================================================================= + + +def _archive_list(): + """List archived sessions.""" + + +def _archive_log(): + """Show an archived log.""" + + +TOP = {"info": echo} +ARCHIVE = {"list": _archive_list, "log": _archive_log} + + +class TestTheGroupKwargsChannel: + """Every rejected spelling names the accepted one.""" + + def test_group_kwargs_as_a_dispatch_keyword_names_the_two_call_form(self): + with pytest.raises(TypeError) as error: + cw.dispatch({"archive": ARCHIVE}, [], group_kwargs={"title": "T"}) + message = str(error.value) + assert "cw.add_commands" in message + assert "group_name='archive'" in message + assert "raise SystemExit(cw.run(parser))" in message + + def test_group_name_and_arghs_old_namespace_spellings_say_the_same_thing(self): + for keyword in ("group_name", "namespace", "namespace_kwargs"): + with pytest.raises(TypeError) as error: + cw.mk_parser({"archive": ARCHIVE}, **{keyword: "archive"}) + assert "cw.add_commands" in str(error.value) + + def test_two_group_keywords_at_once_print_the_recipe_once(self): + """They share one explanation; printing that paragraph twice is worse than once.""" + with pytest.raises(TypeError) as error: + cw.mk_parser({"archive": ARCHIVE}, group_name="archive", group_kwargs={}) + message = str(error.value) + assert message.count("cw's group keywords belong to") == 1 + assert "'group_kwargs'" in message and "'group_name'" in message + + def test_a_funcs_and_kwargs_pair_as_a_mapping_value_is_refused_by_name(self): + """Issue #31's option 1. Without the check this died inside `command_name` + complaining that a dict has no `__name__`.""" + with pytest.raises(cw.CommandTreeError) as error: + cw.mk_parser({"archive": (ARCHIVE, {"title": "T"})}) + message = str(error.value) + assert "(commands, group_kwargs) pair" in message + assert "cw.add_commands" in message + + def test_a_group_title_in_config_is_refused_by_name(self): + """Issue #31's option 3. `config` is per-PARAMETER particulars; a group's + `add_subparsers` keywords are not particulars of any parameter.""" + with pytest.raises(GrammarError) as error: + cw.mk_parser({"archive": ARCHIVE}, config={"archive": {"title": "T"}}) + message = str(error.value) + assert "matches no command" in message + assert "cw.add_commands" in message + + def test_an_ordinary_config_typo_does_not_get_the_group_recipe(self): + """The hint fires only when EVERY unknown key names an `add_subparsers` keyword. + A misspelt command still gets the short, relevant error.""" + with pytest.raises(GrammarError) as error: + cw.mk_parser({"archive": ARCHIVE}, config={"archive": {"lst": {}}}) + assert "cw.add_commands" not in str(error.value) + + def test_an_ordinary_group_still_builds(self): + """The guard must not catch a real group. `[f, g]` is two callables, not a pair.""" + parser = cw.mk_parser({"archive": [_archive_list, _archive_log]}, prog="x") + assert parser.format_usage() == "usage: x [-h] {archive} ...\n" + + def test_the_recipe_in_the_error_message_actually_works(self): + """The `t/xa` shape, executed. The group gets its row in the parent's --help from + `title`, the commands land under it, and the exit code survives. + """ + parser = cw.mk_parser(TOP, prog="xa") + add_commands( + parser, + ARCHIVE, + group_name="archive", + group_kwargs={"title": "Postmortem archive"}, + ) + assert parser.format_usage() == "usage: xa [-h] {info,archive} ...\n" + assert "Postmortem archive" in parser.format_help() + archive = _sub(parser, "archive") + assert _sub(archive, "list") is not None and _sub(archive, "log") is not None + out, err = io.StringIO(), io.StringIO() + assert cw.run(parser, ["info", "hi"], out=out, err=err) == 0 + assert out.getvalue() == "hi\n" + assert cw.run(parser, ["nope"], out=io.StringIO(), err=io.StringIO()) == 2 + + def test_group_kwargs_help_is_inert_and_title_is_not(self): + """The trap the recipe warns about, pinned. argparse ACCEPTS `help` on + `add_subparsers` and renders it nowhere the parent's listing looks -- which is how + `t/xa` shipped a group description nobody ever saw (xa#13). cw reproduces argh + here deliberately; what changed is that the error message now says so. + """ + with_help = cw.mk_parser({}, prog="x") + add_commands(with_help, ARCHIVE, group_name="archive", group_kwargs={"help": "H"}) + assert "H" not in with_help.format_help() + + with_title = cw.mk_parser({}, prog="x") + add_commands(with_title, ARCHIVE, group_name="archive", group_kwargs={"title": "H"}) + assert "H" in with_title.format_help() diff --git a/tests/test_resolution.py b/tests/test_resolution.py index cd22dc5..165e632 100644 --- a/tests/test_resolution.py +++ b/tests/test_resolution.py @@ -88,3 +88,250 @@ def func(apple): with pytest.raises(ImportError, match=r"cw\[resource\]"): resource_inputs(func, resource=dict(apple=None)) + + +# ======================================================================================= +# The uncovered third (cw#32 item 3) +# +# `cw/resolution.py` is pre-existing code with a live dependent (`t/theremin`), and the +# issue is explicit that it should be TESTED, not refactored blind. So these tests are +# characterization: they say what the module does today, error messages included, so that +# the merge the module's own TODOs ask for has a net under it before anyone starts. +# +# Almost every uncovered line was an error path. That is the shape of the risk: a resolver +# whose happy path is exercised by one dependent and whose refusals are exercised by +# nobody is a module where a rewrite can silently start accepting garbage. +# ======================================================================================= + +from cw.resolution import ( # noqa: E402 + _extract_func_name, + _resolve_resource_spec, + parse_json_spec, + parse_spec_with_dot_path, + resolve_func_from_dot_path, + resolve_object, + resolve_to_function, +) + + +class TestResolveFuncFromDotPath: + """Four ways a dot path fails to name a callable, and what each one says.""" + + def test_a_bare_name_that_is_not_a_builtin(self): + with pytest.raises(ValueError, match="as a built-in function"): + resolve_func_from_dot_path("no_such_builtin_anywhere") + + def test_a_builtin_type_attribute_that_does_not_exist(self): + with pytest.raises(ValueError, match=r"'str' has no attribute 'nope'"): + resolve_func_from_dot_path("str.nope") + + def test_a_builtin_type_attribute_that_is_not_callable(self): + with pytest.raises(ValueError, match="is not callable"): + resolve_func_from_dot_path("str.__doc__") + + def test_a_builtin_type_method_resolves(self): + """The happy path of the builtin-type branch. It is asserted by a doctest too, but + a doctest is not a test of this function's coverage under `pytest tests/`.""" + assert resolve_func_from_dot_path("str.upper")("hi") == "HI" + + def test_a_module_attribute_that_is_not_callable(self): + with pytest.raises(ValueError, match=r"'os.sep' is not callable"): + resolve_func_from_dot_path("os.sep") + + def test_a_module_that_does_not_import(self): + with pytest.raises(ValueError, match="Cannot resolve"): + resolve_func_from_dot_path("no_such_module_at_all.thing") + + def test_a_module_attribute_that_does_not_exist(self): + with pytest.raises(ValueError, match="Cannot resolve"): + resolve_func_from_dot_path("os.no_such_attribute") + + +class TestParseSpecWithDotPath: + def test_a_non_string_is_a_type_error(self): + with pytest.raises(TypeError, match="must be a string"): + parse_spec_with_dot_path(123) + + def test_anything_but_word_characters_and_dots_is_refused(self): + """This is the whole of its validation, and it is what keeps `rm -rf` out.""" + with pytest.raises(ValueError, match="word characters and dots"): + parse_spec_with_dot_path("os.system('rm -rf /')") + + +class TestParseJsonSpec: + """Five refusals, one per malformed shape.""" + + @pytest.mark.parametrize( + "spec,match", + [ + ("{not json", "Invalid JSON"), + ("[1, 2]", "must be a dictionary"), + ('{"params": {}}', "must contain 'func' key"), + ('{"func": 3}', "'func' value must be a string"), + ('{"func": "len", "params": 3}', "'params' value must be a dictionary"), + ], + ) + def test_it_says_which_part_is_wrong(self, spec, match): + with pytest.raises(ValueError, match=match): + parse_json_spec(spec) + + def test_params_defaults_to_empty_when_absent(self): + assert parse_json_spec('{"func": "len"}') == ("len", {}) + + +class TestParseAstSpec: + """The safety story: only a call, only keywords, only literals.""" + + def test_a_non_string_is_a_type_error(self): + with pytest.raises(TypeError, match="must be a string"): + parse_ast_spec(123) + + def test_no_parentheses_falls_back_to_the_dot_path_parser(self): + assert parse_ast_spec("os.path.join") == ("os.path.join", {}) + + def test_a_syntax_error_is_reported_as_one(self): + """Note the spec must contain BOTH parentheses to get this far: the `(`/`)` + pre-check routes `'len((('` to the dot-path parser instead, which refuses it for a + different reason. Characterized rather than changed -- both spellings are refused, + and the pre-check is what makes a bare dot path work at all.""" + with pytest.raises(ValueError, match="Invalid syntax"): + parse_ast_spec("f(x=)") + with pytest.raises(ValueError, match="word characters and dots"): + parse_ast_spec("len(((") + + def test_an_expression_that_is_not_a_call_is_refused(self): + with pytest.raises(ValueError, match="must be a function call expression"): + parse_ast_spec("(1 + 2)") + + def test_positional_arguments_are_refused(self): + """Deliberate: a positional has no name to bind to, so only keywords are allowed.""" + with pytest.raises(ValueError, match="Only keyword arguments"): + parse_ast_spec("len([1, 2])") + + def test_double_star_kwargs_are_refused(self): + with pytest.raises(ValueError, match=r"\*\*kwargs syntax not supported"): + parse_ast_spec("f(**d)") + + def test_a_non_literal_argument_value_is_refused(self): + """`ast.literal_eval` is the safety boundary; a name is not a literal.""" + with pytest.raises(ValueError, match="Unsafe or invalid argument value"): + parse_ast_spec("f(x=some_name)") + + def test_a_dotted_function_name_is_rebuilt_from_the_attribute_chain(self): + assert parse_ast_spec("a.b.c(x=1)") == ("a.b.c", {"x": 1}) + + def test_a_function_name_that_is_neither_a_name_nor_an_attribute(self): + import ast as ast_module + + node = ast_module.parse("f()", mode="eval").body.func + assert _extract_func_name(node) == "f" + with pytest.raises(ValueError, match="Unsupported function name format"): + _extract_func_name(ast_module.parse("[f][0]()", mode="eval").body.func) + + +class TestResolveToFunction: + """The entry point: what it accepts, what it binds, and how it refuses.""" + + def test_a_callable_spec_is_returned_untouched(self): + assert resolve_to_function(len) is len + + def test_a_mapping_get_func_is_used_as_a_lookup(self): + """A Mapping is accepted directly, not only its `.get` -- and this branch had no + test, so nothing said whether a dict was allowed at all.""" + assert resolve_to_function("a", parse_ast_spec, {"a": len}) is len + + def test_a_lookup_that_raises_is_reported_with_both_the_key_and_the_spec(self): + def exploding_get(key): + raise KeyError(key) + + with pytest.raises(ValueError, match="could not resolve 'a'"): + resolve_to_function("a()", parse_ast_spec, exploding_get) + + def test_a_lookup_that_returns_a_non_callable_is_a_type_error(self): + with pytest.raises(TypeError, match="returned non-callable object"): + resolve_to_function("a", parse_ast_spec, {"a": 3}) + + def test_kwargs_are_bound_with_partial(self): + resolved = resolve_to_function( + "join(b='B')", parse_ast_spec, {"join": lambda a, b: a + b} + ) + assert resolved("A") == "AB" + + def test_a_parser_that_returns_non_dict_kwargs_is_a_type_error(self): + def bad_parser(spec): + return spec, [("a", 1)] # a list of pairs, not a dict + + with pytest.raises(TypeError, match="kwargs must be a dictionary"): + resolve_to_function("len", bad_parser, {"len": len}) + + def test_anything_that_is_neither_callable_nor_a_string_is_refused(self): + with pytest.raises(TypeError, match="must be either a callable or a string"): + resolve_to_function(42) + + +class TestResolveResourceSpec: + """The three accepted spec shapes, and the refusal of a fourth.""" + + def test_none_means_the_default_ingress(self): + assert _resolve_resource_spec(None, resolve_to_function) is resolve_to_function + + def test_a_callable_is_used_as_is(self): + def resolver(x): + return x # pragma: no cover + + assert _resolve_resource_spec(resolver, resolve_to_function) is resolver + + def test_a_dict_becomes_partial_keywords_on_the_default(self): + resolved = _resolve_resource_spec({"get_func": {"a": len}}, resolve_to_function) + assert resolved("a") is len + + def test_anything_else_says_what_the_three_shapes_are(self): + with pytest.raises(TypeError, match="must be None, callable, or dict"): + _resolve_resource_spec(42, resolve_to_function) + + +class TestResolveObject: + """`resolve_object` is deliberately NOT at the package facade (ADR-0007), and it had no + test at all. It stays where it is; these tests are what make the merge its own TODO + asks for reviewable rather than a rewrite from scratch. + """ + + MAP = {"a": 1, "b": "two"} + + def test_a_string_is_looked_up(self): + assert resolve_object("a", object_map=self.MAP) == 1 + + def test_a_string_that_is_not_in_the_map_is_a_value_error(self): + with pytest.raises(ValueError, match="Unknown object identifier: zz"): + resolve_object("zz", object_map=self.MAP) + + def test_a_custom_error_message_replaces_the_default(self): + with pytest.raises(ValueError, match="pick one of a, b"): + resolve_object("zz", object_map=self.MAP, error_message="pick one of a, b") + + def test_a_non_string_passes_through_when_no_type_is_expected(self): + sentinel = object() + assert resolve_object(sentinel, object_map=self.MAP) is sentinel + + def test_a_non_string_of_the_expected_type_passes_through(self): + assert resolve_object(7, object_map=self.MAP, expected_type=int) == 7 + + def test_a_non_string_of_the_wrong_type_is_a_type_error(self): + with pytest.raises(TypeError, match="Expected type"): + resolve_object(7.5, object_map=self.MAP, expected_type=int) + + def test_a_looked_up_value_of_the_wrong_type_is_also_a_type_error(self): + """The second type check, on the *resolved* value, is the one a reader misses: a + map entry may be the wrong type even when the key was fine.""" + with pytest.raises(TypeError, match="Resolved object should be of type"): + resolve_object("b", object_map=self.MAP, expected_type=int) + + def test_the_custom_message_covers_both_type_errors(self): + with pytest.raises(TypeError, match="nope"): + resolve_object( + 7.5, object_map=self.MAP, expected_type=int, error_message="nope" + ) + with pytest.raises(TypeError, match="nope"): + resolve_object( + "b", object_map=self.MAP, expected_type=int, error_message="nope" + ) diff --git a/tests/test_testing.py b/tests/test_testing.py index 8854ac8..805f7fa 100644 --- a/tests/test_testing.py +++ b/tests/test_testing.py @@ -24,6 +24,7 @@ #: it is the reason the file can be copied into a repo that will never depend on cw. ALLOWED_MODULE_IMPORTS = { "argparse", + "contextlib", "difflib", "json", "os",