From f12088d1d8ab2497bee05e56fa2bac7320891e5a Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Wed, 2 Sep 2026 23:09:39 +0200 Subject: [PATCH 1/2] modern_decode: read abstract sequence interfaces as several values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `argh` recognises `list` and nothing else, so a parameter annotated `names: Sequence[str]` became ONE positional taking ONE token. The failure is quiet in the way that costs time: the parser builds, `--help` looks right, the command runs, and the second argument comes back as an "unrecognized argument" pointing at the value rather than at the annotation. That penalises exactly the annotation style that prefers `collections.abc` interfaces over concrete containers — a signature written `Sequence[str]` means the same thing to a reader as `list[str]` and meant something else to the parser. `modern_decode` now covers `Sequence` / `MutableSequence` / `Iterable` / `Collection` / `Set` / `MutableSet`, plus `tuple` / `set` / `frozenset`, subscripted or bare. `argh_decode` is **untouched**: it is argh's if-chain if-branch for if-branch, and widening it would break the compatibility contract that is its whole purpose. A test asserts it still returns `{}` for every new spelling, and still returns what it always did for `list[int]`. Three decisions in the implementation: - **`str` and `bytes` must never be read as sequences**, and matching on `typing.get_origin` avoids it BY CONSTRUCTION rather than by a special case: `get_origin(str)` is None. Both are registered `Sequence`s, so an `issubclass` test would have read every string parameter as variadic and broken every existing CLI. Tested directly, because it is the trap this change exists inside. - **A homogeneous fixed-length tuple gets a fixed `nargs`** — `tuple[int, int]` -> `nargs=2, type=int`. It is the one case where the annotation carries a count, and it makes `--size W H` fall out of the signature. - **A heterogeneous tuple gets no inference at all.** `add_argument` has a single `type` and there is no honest one to choose, so `tuple[int, str]` falls through to `{}` rather than guessing one and converting the other wrongly. Found while adopting `cw` as the CLI for `looks` (t/looks), whose house style annotates with `collections.abc` interfaces throughout. Claude-Session: https://claude.ai/code/session_01CnXLihXKG9jfvygHvgKVpX --- cw/grammar.py | 107 ++++++++++++++++++++++++++++++++++++++++++ tests/test_grammar.py | 77 ++++++++++++++++++++++++++++++ 2 files changed, 184 insertions(+) diff --git a/cw/grammar.py b/cw/grammar.py index f56b529..f0d4c2e 100644 --- a/cw/grammar.py +++ b/cw/grammar.py @@ -39,6 +39,7 @@ :class:`cw.base.ArghHelpFormatter` renders as ``repr(default)``. """ +import collections.abc import dataclasses import enum import inspect @@ -101,6 +102,46 @@ _NONE_TYPE = type(None) +#: Origins :func:`modern_decode` reads as "several values", beyond ``list``. +#: +#: ``argh`` recognises ``list`` and nothing else, so ``names: Sequence[str]`` +#: silently becomes one positional taking one token. That penalises exactly the +#: annotation style that prefers ``collections.abc`` interfaces over concrete +#: containers — and the failure is quiet: the parser builds, the command runs, +#: and the second argument is an "unrecognized argument". +#: +#: ``str`` and ``bytes`` are *not* reachable here and must never become so. Both +#: are registered ``Sequence``\ s, so an ``issubclass`` test would read every +#: ``name: str`` as variadic. Matching on ``typing.get_origin`` avoids that by +#: construction: ``get_origin(str)`` is ``None``. +_SEQUENCE_ORIGINS: tuple = ( + list, + set, + frozenset, + tuple, + collections.abc.Sequence, + collections.abc.MutableSequence, + collections.abc.Iterable, + collections.abc.Collection, + collections.abc.Set, + collections.abc.MutableSet, +) + +#: Bare, unsubscripted spellings of the same idea: ``x: Sequence`` rather than +#: ``x: Sequence[str]``. ``list`` is absent because :func:`argh_decode` already +#: covers it, and this table must not change what argh-compatible mode does. +_BARE_SEQUENCES: tuple = ( + set, + frozenset, + tuple, + collections.abc.Sequence, + collections.abc.MutableSequence, + collections.abc.Iterable, + collections.abc.Collection, + collections.abc.Set, + collections.abc.MutableSet, +) + class GrammarError(ValueError): """A signature and its overrides cannot be reconciled into a CLI. @@ -382,6 +423,14 @@ def modern_decode(param: inspect.Parameter, hint: Any) -> Mapping[str, Any]: >>> decoded['metavar'] '{RED}' + + Abstract sequence interfaces mean what they say, which argh-compatible mode + cannot do — argh recognises ``list`` and nothing else: + + >>> modern_decode(p, typing.Sequence[str]) == {'nargs': '*', 'type': str} + True + >>> argh_decode(p, typing.Sequence[str]) + {} """ hint = _unwrap_optional(hint) if isinstance(hint, type) and issubclass(hint, enum.Enum): @@ -396,9 +445,67 @@ def modern_decode(param: inspect.Parameter, hint: Any) -> Mapping[str, Any]: } if isinstance(hint, type) and issubclass(hint, pathlib.PurePath): return {"type": pathlib.Path} + sequence = _decode_sequence(hint) + if sequence is not None: + return sequence return argh_decode(param, hint) +def _decode_sequence(hint: Any) -> Optional[Dict[str, Any]]: + """``Sequence[str]`` and friends -> ``nargs``, or ``None`` to fall through. + + ``argh_decode`` covers ``list`` because argh does; this covers the abstract + interfaces and the other concrete containers, because a signature written + ``names: Sequence[str]`` means the same thing to a reader and currently + means something else to the parser. + + A **homogeneous fixed-length tuple** becomes a fixed ``nargs``, which is the + one case where the annotation carries a count: + + >>> _decode_sequence(tuple[int, int]) == {'nargs': 2, 'type': int} + True + + A heterogeneous one falls through to no inference, because ``add_argument`` + has a single ``type`` and there is no honest one to choose: + + >>> _decode_sequence(tuple[int, str]) is None + True + + The abstract interfaces, subscripted and bare: + + >>> _decode_sequence(typing.Sequence[str]) == {'nargs': '*', 'type': str} + True + >>> _decode_sequence(collections.abc.Iterable) == {'nargs': '*'} + True + + And the trap this must not fall into — ``str`` and ``bytes`` are registered + ``Sequence``\ s, so a subclass test would read every string parameter as + variadic: + + >>> _decode_sequence(str) is None + True + >>> _decode_sequence(bytes) is None + True + """ + if hint in _BARE_SEQUENCES: + return {"nargs": ZERO_OR_MORE} + origin = typing.get_origin(hint) + if origin is None or origin not in _SEQUENCE_ORIGINS: + return None + args = typing.get_args(hint) + if origin is tuple and args and Ellipsis not in args: + # A fixed-length tuple states its own count, but only a homogeneous one + # has a `type` argparse could apply to every position. + if len(set(args)) != 1 or args[0] not in BASIC_TYPES: + return None + return {"nargs": len(args), "type": args[0]} + guessed: Dict[str, Any] = {"nargs": ZERO_OR_MORE} + item = next((a for a in args if a is not Ellipsis), None) + if item in BASIC_TYPES: + guessed["type"] = item + return guessed + + def _unwrap_optional(hint: Any) -> Any: """``Optional[X]`` -> ``X``; anything else unchanged.""" if any(typing.get_origin(hint) is union for union in _UNION_TYPES): diff --git a/tests/test_grammar.py b/tests/test_grammar.py index 4a0b756..f294177 100644 --- a/tests/test_grammar.py +++ b/tests/test_grammar.py @@ -11,6 +11,7 @@ import functools import inspect import pathlib +import collections.abc import typing import pytest @@ -334,6 +335,82 @@ def test_modern_decode_reads_an_enum_by_name_then_value(): decoded["type"]("green") +# -------------------------------------------------------------------------------------- +# Abstract sequence interfaces +# +# argh recognises `list` and nothing else, so `names: Sequence[str]` silently became one +# positional taking one token — the parser built, the command ran, and the second argument +# came back as an "unrecognized argument". That penalises exactly the annotation style +# that prefers `collections.abc` interfaces over concrete containers. + + +@pytest.mark.parametrize( + "hint", + [ + typing.Sequence[str], + typing.Iterable[str], + collections.abc.Sequence[str], + collections.abc.Iterable[str], + collections.abc.Collection[str], + tuple[str, ...], + set[str], + frozenset[str], + ], +) +def test_modern_decode_reads_abstract_sequences_as_several_values(hint): + assert modern_decode(PARAM, hint) == {"nargs": "*", "type": str} + + +@pytest.mark.parametrize( + "hint", + [typing.Sequence, collections.abc.Iterable, tuple, set, frozenset], +) +def test_modern_decode_reads_bare_sequences_too(hint): + """`x: Sequence` says the same thing as `x: Sequence[str]` about arity.""" + assert modern_decode(PARAM, hint) == {"nargs": "*"} + + +@pytest.mark.parametrize("hint", [str, bytes]) +def test_modern_decode_does_NOT_read_str_or_bytes_as_a_sequence(hint): + """The trap. Both are registered `Sequence`s, so an `issubclass` test would + read every string parameter as variadic — and every existing CLI would break + in a way whose error message points at the wrong thing. + + Matching on `typing.get_origin` avoids it by construction: `get_origin(str)` + is None. + """ + assert "nargs" not in modern_decode(PARAM, hint) + + +def test_modern_decode_gives_a_homogeneous_fixed_tuple_a_fixed_nargs(): + """The one case where the annotation carries a count.""" + assert modern_decode(PARAM, tuple[int, int]) == {"nargs": 2, "type": int} + + +def test_modern_decode_declines_a_heterogeneous_tuple(): + """`add_argument` has a single `type`, and there is no honest one to pick — + so no inference at all beats a wrong converter.""" + assert modern_decode(PARAM, tuple[int, str]) == {} + + +def test_modern_decode_unwraps_an_optional_sequence(): + assert modern_decode(PARAM, typing.Optional[typing.Sequence[int]]) == { + "nargs": "*", + "type": int, + } + + +def test_argh_decode_is_UNCHANGED_by_any_of_this(): + """`argh_decode` is argh's if-chain, if-branch for if-branch. Widening it + would break the compatibility contract that is its whole purpose — the new + behaviour lives in `modern_decode`, which is the seam for exactly this.""" + assert argh_decode(PARAM, typing.Sequence[str]) == {} + assert argh_decode(PARAM, collections.abc.Iterable[str]) == {} + assert argh_decode(PARAM, tuple[int, int]) == {} + # and what it DID cover still works + assert argh_decode(PARAM, list[int]) == {"nargs": "*", "type": int} + + def test_modern_decode_maps_pure_paths_to_path(): assert modern_decode(PARAM, pathlib.Path) == {"type": pathlib.Path} assert modern_decode(PARAM, pathlib.PurePosixPath) == {"type": pathlib.Path} From 006bd542da9de271117d1319b1637025b202a428 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Wed, 2 Sep 2026 23:09:39 +0200 Subject: [PATCH 2/2] Format two test files ruff had drifted from Pure line-wrapping and quote normalisation, no semantics. They were already unformatted before this branch; `ruff format` picked them up while formatting the grammar change, and they are split out here so the functional commit contains only the functional change. Claude-Session: https://claude.ai/code/session_01CnXLihXKG9jfvygHvgKVpX --- tests/test_cli.py | 8 ++++++-- tests/test_testing.py | 11 +++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index 05548bc..7634ba0 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -773,9 +773,13 @@ def test_group_kwargs_help_is_inert_and_title_is_not(self): 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"}) + 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"}) + add_commands( + with_title, ARCHIVE, group_name="archive", group_kwargs={"title": "H"} + ) assert "H" in with_title.format_help() diff --git a/tests/test_testing.py b/tests/test_testing.py index 805f7fa..984ae39 100644 --- a/tests/test_testing.py +++ b/tests/test_testing.py @@ -418,10 +418,10 @@ class TestPinnedStdin: """ READS_STDIN = ( - 'import sys\n' - 'try:\n' + "import sys\n" + "try:\n" ' line = input("prompt> ")\n' - 'except EOFError:\n' + "except EOFError:\n" ' line = ""\n' 'print("read:", line)\n' ) @@ -530,7 +530,10 @@ def test_only_the_program_s_own_exe_is_scrubbed(self): def test_the_program_need_not_be_the_command_s_first_word(self): command = ["/usr/bin/python", "/tmp/toy.exe"] - assert testing.scrub_exe_suffix("usage: toy.EXE [-h]", command) == "usage: toy [-h]" + assert ( + testing.scrub_exe_suffix("usage: toy.EXE [-h]", command) + == "usage: toy [-h]" + ) class TestExitStatus: