Close the v1 follow-ups: #31 decided (no fourth channel), #32 fixed

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** —

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,25 +444,51 @@ 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.
 
     This is the rule that closes the trap where ``convention=cw.MODERN`` renames a group
     (``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

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:

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,