diff --git a/CHANGELOG.md b/CHANGELOG.md index f5285c7de..1001a91ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,11 +1,28 @@ ## 4.2.0 (TBD) +- Enhancements + - `@with_annotated` argument groups can now contain an `ArgumentBlock`'s arguments. A `Group` + member names a command-line argument, and a block expands into one argument per field, so its + fields are named: `Group("host", "port")`. +- Breaking Changes + - A `Group` member now names an argument rather than a parameter. The two differ only for an + `ArgumentBlock` parameter, which is expanded away and has no argument of its own: + `Group("conn")` now raises `ValueError` pointing at the block's fields. It previously produced + an empty argument group in `groups=` and a `KeyError` in `mutually_exclusive_groups=`. + - Whether a `Group` member exists is now validated when the parser is built rather than at + decoration time, because a block's field names cannot be known without resolving its type + hint, and resolving hints eagerly would break forward-referenced annotations. A typo in a + member name still raises `ValueError`, but on first use of that command rather than at class + definition. The spec-shape rules (a member listed twice, a member in two groups, + `required=True` on a plain group, the mutex nesting rules) are unaffected and still hard-fail + at decoration time. - Bug Fixes - Fixed `@with_annotated(base_command=True)` not listing its subcommands under the positional arguments section of the parent command's `--help`, unlike `argparse` and `Cmd2ArgumentParser`. They were placed in an untitled section of their own instead. Passing `subcommand_title` or `subcommand_description` still gives the subcommands a dedicated section ([#1715](https://github.com/python-cmd2/cmd2/issues/1715)). + - Fix `@with_annotated` decorator so using `ArgumentBlock` works with groups ## 4.1.2 (July 16, 2026) diff --git a/cmd2/annotated.py b/cmd2/annotated.py index b059c8c5a..cf54251bc 100644 --- a/cmd2/annotated.py +++ b/cmd2/annotated.py @@ -552,25 +552,20 @@ def __init__( ) -> None: """Initialise an argument group definition. - :param members: parameter names to place in the group (at least one) + :param members: argument names to place in the group (at least one). A plain parameter's name is its + argument name; for an ``ArgumentBlock`` name its fields. :param title: group title shown as a help section header :param description: group description shown under the title :param required: ``mutually_exclusive_groups`` only -- require exactly one member. On a ``groups=`` entry it raises ``ValueError``. """ if not members: - raise ValueError("Group requires at least one member parameter name") + raise ValueError("Group requires at least one member argument name") self.members = members self.title = title self.description = description self.required = required - def _validate_members(self, *, all_param_names: set[str], group_type: str) -> None: - """Validate that every referenced member parameter exists.""" - for name in self.members: - if name not in all_param_names: - raise ValueError(f"{group_type} references nonexistent parameter {name!r}") - #: Metadata extracted from ``Annotated[T, meta]``, or ``None`` for plain types. ArgMetadata = Argument | Option | None @@ -2072,6 +2067,39 @@ def _shared_field_dest(dc_type: type, field_name: str) -> str: return constants.cmd2_private_attr_name(f"shared_{id(dc_type):x}_{field_name}") +def _check_group_members_exist( + by_name: dict[str, _ArgparseArgument], + block_param_names: set[str], + func_qualname: str, + *specs: tuple[Group, ...] | None, +) -> None: + """Reject a ``Group`` member that names no command-line argument, once every argument is built. + + :func:`_validate_group_specs` cannot do this at decoration time: an ArgumentBlock's arguments are its + dataclass fields, which only resolving the block's type hint would reveal, and resolving hints there + would break forward-referenced annotations. So this is where a bad member name is finally caught. + + Naming the block parameter itself is the likely mistake -- it reads natural but the parameter is expanded + away and has no argument -- so it gets its own message pointing at the fields to name instead. + """ + for spec_group in specs: + for spec in spec_group or (): + for name in spec.members: + if name in by_name: + continue + if name in block_param_names: + raise ValueError( + f"group in {func_qualname} references {name!r}, which is an ArgumentBlock parameter " + f"and not a command-line argument: a block is expanded into one argument per field, " + f"so name those fields instead." + ) + raise ValueError( + f"group in {func_qualname} references {name!r}, which is not a command-line argument. " + f"Members name arguments: a plain parameter is its own argument, and an ArgumentBlock's " + f"arguments are its field names." + ) + + def _link_mutex_group_membership( by_name: dict[str, _ArgparseArgument], mutually_exclusive_groups: tuple[Group, ...] | None, @@ -2079,8 +2107,7 @@ def _link_mutex_group_membership( """Append each mutex group's 1-based index to its member arguments' ``mutex_group_indices``. This membership is the fact behind the required-member constraint row. Member references are - validated upstream by :func:`_validate_group_specs` before this runs, so every member name - resolves to a built argument. + resolved to built arguments upstream by :func:`_check_group_members_exist` before this runs. """ if not mutually_exclusive_groups: return @@ -2352,6 +2379,7 @@ def _resolve_parameters( *, skip_params: frozenset[str] = _SKIP_PARAMS, base_command: bool = False, + groups: tuple[Group, ...] | None = None, mutually_exclusive_groups: tuple[Group, ...] | None = None, ) -> tuple[list[_ArgparseArgument], set[type]]: """Resolve a function signature into argparse-argument builders and inheritable-block types. @@ -2378,6 +2406,7 @@ def _resolve_parameters( resolved: list[_ArgparseArgument] = [] inherited_field_names: set[str] = set() base_args_types: set[type] = set() + block_param_names: set[str] = set() # Skip the first parameter by position (self/cls for methods) params = list(sig.parameters.items()) @@ -2398,6 +2427,7 @@ def _resolve_parameters( f"cannot have a default value; remove the default." ) inherited_field_names.update(_init_field_names(inherited)) + block_param_names.add(name) continue # An ArgumentBlock-typed parameter is an argument block: expand its fields in place (flat) instead of @@ -2416,6 +2446,7 @@ def _resolve_parameters( block_hint, func_qualname=func.__qualname__, base_command=base_command, shared=name == NS_ATTR_BASE_ARGS ) _reject_field_shadowing_block_param(block_hint, name, func.__qualname__) + block_param_names.add(name) resolved.extend(expanded) continue # The magic name requires a bare ArgumentBlock; a non-block annotation on it is a clear mistake. @@ -2480,6 +2511,7 @@ def _resolve_parameters( for arg in positionals[:-1]: # every positional except the last has a following positional arg.has_following_positional = True by_name = {arg.name: arg for arg in resolved} + _check_group_members_exist(by_name, block_param_names, func.__qualname__, groups, mutually_exclusive_groups) _link_mutex_group_membership(by_name, mutually_exclusive_groups) for arg in resolved: arg._check_constraints() @@ -2539,30 +2571,22 @@ def _filtered_namespace_kwargs( def _validate_group_specs( - func: Callable[..., Any], - *, - skip_params: frozenset[str], groups: tuple[Group, ...] | None, mutually_exclusive_groups: tuple[Group, ...] | None, ) -> None: - """Validate ``groups=`` / ``mutually_exclusive_groups=`` specs from parameter names alone. - - Runs at decoration time (from both the regular-command and subcommand decoration paths, and - again from :func:`build_parser_from_function` for direct callers), so a misconfigured group - hard-fails when the class is defined instead of on first command use, where cmd2's runtime - handler turns the error into a printed message. Reads only parameter names and the ``Group`` - specs -- never the type hints -- so forward-referenced annotations still decorate. The one - group rule that needs the annotations (a required member in a mutually exclusive group) stays - in :data:`_CONSTRAINTS` and fires when the parser is built. + """Validate the *shape* of ``groups=`` / ``mutually_exclusive_groups=`` specs. + + Runs at decoration time (from both the regular-command and subcommand decoration paths, and again from + :func:`build_parser_from_function` for direct callers), so a misshapen group hard-fails when the class is + defined instead of on first command use, where cmd2's runtime handler turns the error into a printed + message. Reads only the ``Group`` specs -- never the signature or the type hints -- so + forward-referenced annotations still decorate. """ if not groups and not mutually_exclusive_groups: return - params = list(inspect.signature(func).parameters)[1:] # skip self/cls by position - all_param_names = {name for name in params if name not in skip_params} group_entry_for: dict[str, int] = {} for index, spec in enumerate(groups or (), start=1): - spec._validate_members(all_param_names=all_param_names, group_type="groups") if spec.required: raise ValueError( "Group(required=True) is only valid in mutually_exclusive_groups; " @@ -2580,7 +2604,6 @@ def _validate_group_specs( mutex_entry_for: dict[str, int] = {} for index, spec in enumerate(mutually_exclusive_groups or (), start=1): - spec._validate_members(all_param_names=all_param_names, group_type="mutually_exclusive_groups") for name in spec.members: previous = mutex_entry_for.get(name) if previous == index: @@ -2721,7 +2744,7 @@ def build_parser_from_function( from . import argparse_utils # The decorator already ran this at decoration time; direct callers get the same checks here. - _validate_group_specs(func, skip_params=skip_params, groups=groups, mutually_exclusive_groups=mutually_exclusive_groups) + _validate_group_specs(groups, mutually_exclusive_groups) parser_cls = parser_class or argparse_utils.DEFAULT_ARGUMENT_PARSER if "description" not in parser_kwargs: @@ -2735,6 +2758,7 @@ def build_parser_from_function( resolved, base_args_types = _resolve_parameters( func, skip_params=skip_params, + groups=groups, mutually_exclusive_groups=mutually_exclusive_groups, ) @@ -2864,12 +2888,7 @@ def _build_subcommand_handler( # Validate the group specs eagerly (decoration time) so a misconfigured group hard-fails when # the class is defined; the name-only checks never resolve type hints, so forward-referenced # annotations still decorate and the parser build stays deferred. - _validate_group_specs( - func, - skip_params=_SKIP_PARAMS, - groups=options.groups, - mutually_exclusive_groups=options.mutually_exclusive_groups, - ) + _validate_group_specs(options.groups, options.mutually_exclusive_groups) if base_command: # Validate eagerly (decoration time); the base-command rows in _CONSTRAINTS fire here. # skip_params is spelled out so this call cannot silently diverge from the parser build below. @@ -3053,12 +3072,7 @@ def decorator(fn: Callable[..., Any]) -> Callable[..., Any]: # Validate the group specs eagerly (decoration time) so a misconfigured group hard-fails when # the class is defined; the name-only checks never resolve type hints, so forward-referenced # annotations still decorate and the parser build stays deferred. - _validate_group_specs( - fn, - skip_params=skip_params, - groups=options.groups, - mutually_exclusive_groups=options.mutually_exclusive_groups, - ) + _validate_group_specs(options.groups, options.mutually_exclusive_groups) if base_command: # Validate eagerly (decoration time); the base-command rows in _CONSTRAINTS fire here. _resolve_parameters(fn, skip_params=skip_params, base_command=True) diff --git a/docs/features/annotated.md b/docs/features/annotated.md index 6895a03ec..98d087a97 100644 --- a/docs/features/annotated.md +++ b/docs/features/annotated.md @@ -445,8 +445,8 @@ token to convert. Both raise `TypeError` at decoration time. - `help` -- help text for an annotated subcommand (only valid with `subcommand_to`) - `aliases` -- aliases for an annotated subcommand (only valid with `subcommand_to`) - `deprecated` -- mark the subcommand as deprecated in `--help` (only valid with `subcommand_to`) -- `groups` -- `Group` instances assigning parameter names to argument groups -- `mutually_exclusive_groups` -- `Group` instances of mutually exclusive parameters +- `groups` -- `Group` instances assigning argument names to argument groups +- `mutually_exclusive_groups` -- `Group` instances of mutually exclusive arguments - `parser_class` -- a custom parser class (defaults to the configured default) - `**parser_kwargs` -- every other `Cmd2ArgumentParser` constructor kwarg, forwarded through PEP 692 [`Unpack[Cmd2ParserKwargs]`][cmd2.annotated.Cmd2ParserKwargs]. See @@ -549,13 +549,35 @@ both places, a mutex that sits only partly in a `groups=` entry, or one that spa raises `ValueError`. The other three nestings (an argument group inside another group or a mutex, or a mutex inside a mutex) are removed in argparse on Python 3.14 and cannot be expressed here. -All of these group-spec rules -- member references, a parameter assigned to two groups, +A `Group` member names a command-line **argument**. For a plain parameter the argument is the +parameter, so the name is the same either way. An `ArgumentBlock` parameter is different: it is +expanded into one argument per field (field name == argument name), so you name its fields rather +than the block: + +```python +@dataclass +class Conn(ArgumentBlock): + host: Annotated[str, Option("--host")] = "localhost" + port: Annotated[int, Option("--port")] = 8080 + +@with_annotated(groups=(Group("host", "port", title="connection"),)) +def do_connect(self, conn: Conn) -> None: ... +``` + +Naming the block parameter itself (`Group("conn")`) raises `ValueError`: the parameter is expanded +away and has no argument of its own. Naming a block's fields individually is also what lets you say +which fields are alternatives -- put `Group("json", "csv")` in `mutually_exclusive_groups` and only +those two exclude each other, rather than every field of the block they happen to live in. + +The group-spec **shape** rules -- a member listed twice, a member assigned to two groups, `required=True` on a plain group, and the mutex nesting rules above -- are validated when the -decorator runs, so a misconfigured group raises `ValueError` at class definition time instead of on -first command use. The checks read only parameter names, never the type hints, so forward-referenced -annotations still decorate cleanly. The one group rule that depends on the annotations (a member of -a mutually exclusive group must be omittable -- have a default or be `T | None`) fires when the -parser is built. +decorator runs, so a misshapen group raises `ValueError` at class definition time instead of on +first command use. Those checks read only the `Group` specs, never the signature or the type hints, +so forward-referenced annotations still decorate cleanly. + +The rules that depend on what a member _is_ fire when the parser is built, because a block's field +names cannot be known without resolving its type hint: that every member names a real argument, and +that a member of a mutually exclusive group is omittable (has a default or is `T | None`). `parents=` mirrors argparse's standard parents mechanism for sharing argument definitions across parsers. `argument_default=argparse.SUPPRESS` is not supported and raises `TypeError`. It removes an diff --git a/tests/test_annotated.py b/tests/test_annotated.py index 6ed8f15e3..6f39699f7 100644 --- a/tests/test_annotated.py +++ b/tests/test_annotated.py @@ -1034,7 +1034,7 @@ def test_groups_and_mutex_applied(self) -> None: assert {"force", "dry_run"} in mutex_groups def test_group_nonexistent_param_raises(self) -> None: - with pytest.raises(ValueError, match="nonexistent parameter"): + with pytest.raises(ValueError, match="'missing', which is not a command-line argument"): build_parser_from_function(_func_grouped, groups=(Group("missing"),)) def test_param_in_multiple_groups_raises(self) -> None: @@ -1178,7 +1178,7 @@ def test_nonexistent_member_reported_over_required_member(self) -> None: def func(self, local: Annotated[str, Option("--local")], remote: str | None = None) -> None: ... - with pytest.raises(ValueError, match=r"nonexistent parameter 'ghost'"): + with pytest.raises(ValueError, match=r"'ghost', which is not a command-line argument"): build_parser_from_function(func, mutually_exclusive_groups=(Group("local", "ghost"),)) def test_argument_group(self) -> None: @@ -1376,10 +1376,6 @@ def team_add(self, name: str) -> None: class TestGroupHelpers: - def test_validate_group_members_rejects_nonexistent_param(self) -> None: - with pytest.raises(ValueError, match="nonexistent"): - Group("verbose", "nonexistent")._validate_members(all_param_names={"verbose"}, group_type="groups") - def test_build_argument_group_targets(self) -> None: parser = argparse.ArgumentParser() target_for, argument_group_for = _build_argument_group_targets(parser, groups=(Group("src", "dst"),)) @@ -1436,33 +1432,154 @@ def func(self, src: str = "a", dst: str = "b") -> None: ... with pytest.raises(ValueError, match="different argument groups"): _validate_group_specs( - func, - skip_params=frozenset(), groups=(Group("src"), Group("dst")), mutually_exclusive_groups=(Group("src", "dst"),), ) -class TestEagerGroupSpecValidation: - """Group specs hard-fail at decoration time (class definition), not on first command use. +class TestBuildTimeMemberValidation: + """A Group member names an argument, and that is checked once the arguments are built. - The decorator runs the name-only spec checks before deferring the parser build, so a - misconfigured group raises while the class body executes instead of surfacing as a swallowed - runtime error the first time the command runs. Type hints are never resolved by these checks, - so forward-referenced annotations still decorate (the parser build stays deferred for them). + It cannot be checked at decoration time: an ArgumentBlock expands into one argument per field, and the + field names live inside the dataclass, reachable only by resolving the block's type hint -- which the + decoration-time checks never do, so forward references keep working. """ - def test_member_typo_fails_at_decoration(self) -> None: - def do_x(self, a: str = "v") -> None: ... + def test_block_fields_can_be_named_in_an_argument_group(self) -> None: + """The #1714 case: a block's arguments are its fields, so its fields are what a group names.""" + + @dataclass + class Conn(ArgumentBlock): + host: Annotated[str, Option("--host")] = "localhost" + port: Annotated[int, Option("--port")] = 8080 + + def func(self, conn: Conn, verbose: bool = False) -> None: ... + + parser = build_parser_from_function(func, groups=(Group("host", "port", title="Connection"),)) + section = next(g for g in parser._action_groups if g.title == "Connection") + assert {a.dest for a in section._group_actions} == {"host", "port"} + + def test_block_fields_can_be_named_in_a_mutex_group(self) -> None: + """Fields that are genuinely alternatives are named individually, so the meaning is explicit.""" + + @dataclass + class Format(ArgumentBlock): + json: Annotated[bool, Option("--json")] = False + csv: Annotated[bool, Option("--csv")] = False + + def func(self, fmt: Format, verbose: bool = False) -> None: ... + + parser = build_parser_from_function(func, mutually_exclusive_groups=(Group("json", "csv"),)) + assert {a.dest for a in parser._mutually_exclusive_groups[0]._group_actions} == {"json", "csv"} + with pytest.raises(SystemExit): + parser.parse_args(["--json", "--csv"]) + + def test_a_group_can_mix_block_fields_and_plain_parameters(self) -> None: + """Members name arguments, so where an argument came from stops mattering once it is built. + + The block's own `port` is left out to pin down that a group takes the fields it names rather + than the whole block that one of them happens to belong to. + """ + + @dataclass + class Conn(ArgumentBlock): + host: Annotated[str, Option("--host")] = "localhost" + port: Annotated[int, Option("--port")] = 8080 + + def func(self, conn: Conn, verbose: bool = False) -> None: ... + + parser = build_parser_from_function(func, groups=(Group("host", "verbose", title="Mixed"),)) + section = next(g for g in parser._action_groups if g.title == "Mixed") + assert {a.dest for a in section._group_actions} == {"host", "verbose"} + ns = parser.parse_args(["--host", "h", "--verbose"]) + assert (ns.host, ns.verbose) == ("h", True) + + def test_a_mutex_can_mix_a_block_field_and_a_plain_parameter(self) -> None: + """A block field and a plain parameter can be genuine alternatives to each other.""" + + @dataclass + class Conn(ArgumentBlock): + host: Annotated[str, Option("--host")] = "localhost" + + def func(self, conn: Conn, verbose: bool = False) -> None: ... + + parser = build_parser_from_function(func, mutually_exclusive_groups=(Group("host", "verbose"),)) + assert {a.dest for a in parser._mutually_exclusive_groups[0]._group_actions} == {"host", "verbose"} + assert parser.parse_args(["--host", "h"]).host == "h" + assert parser.parse_args(["--verbose"]).verbose is True + with pytest.raises(SystemExit): + parser.parse_args(["--host", "h", "--verbose"]) + + def test_naming_the_block_parameter_is_rejected(self) -> None: + """The block parameter is expanded away and has no argument, so it cannot be a member.""" + + @dataclass + class Conn(ArgumentBlock): + host: Annotated[str, Option("--host")] = "localhost" + + def func(self, conn: Conn, verbose: bool = False) -> None: ... + + with pytest.raises(ValueError, match="'conn', which is an ArgumentBlock parameter"): + build_parser_from_function(func, groups=(Group("conn", title="Connection"),)) + + def test_naming_the_block_parameter_in_a_mutex_is_rejected_too(self) -> None: + """Same rule in both spec kinds -- there is no groups=/mutex asymmetry to remember.""" + + @dataclass + class Conn(ArgumentBlock): + host: Annotated[str, Option("--host")] = "localhost" + + def func(self, conn: Conn, verbose: bool = False) -> None: ... + + with pytest.raises(ValueError, match="'conn', which is an ArgumentBlock parameter"): + build_parser_from_function(func, mutually_exclusive_groups=(Group("conn", "verbose"),)) - with pytest.raises(ValueError, match="groups references nonexistent parameter 'typo'"): - with_annotated(groups=(Group("typo"),))(do_x) + def test_naming_the_block_class_is_rejected(self) -> None: + """The class name is not a parameter or an argument; only fields are.""" + + @dataclass + class Conn(ArgumentBlock): + host: Annotated[str, Option("--host")] = "localhost" + + def func(self, conn: Conn, verbose: bool = False) -> None: ... + + with pytest.raises(ValueError, match="'Conn', which is not a command-line argument"): + build_parser_from_function(func, groups=(Group("Conn", title="Connection"),)) + + def test_parent_args_block_parameter_is_rejected(self) -> None: + """cmd2_parent_args declares no arguments of its own; its fields come from the parent.""" + + @dataclass + class Base(ArgumentBlock): + dry: Annotated[bool, Option("--dry")] = False + + def func(self, cmd2_parent_args: Base, verbose: bool = False) -> None: ... + + with pytest.raises(ValueError, match="'cmd2_parent_args', which is an ArgumentBlock parameter"): + build_parser_from_function(func, groups=(Group("cmd2_parent_args", title="Inherited"),)) + + def test_member_typo_fails_at_build(self) -> None: + """A typo still hard-fails -- at the parser build rather than at decoration.""" - def test_mutex_member_typo_fails_at_decoration(self) -> None: def do_x(self, a: str = "v") -> None: ... - with pytest.raises(ValueError, match="mutually_exclusive_groups references nonexistent parameter 'typo'"): - with_annotated(mutually_exclusive_groups=(Group("typo"),))(do_x) + with_annotated(groups=(Group("typo"),))(do_x) # decorates: existence is not knowable yet + with pytest.raises(ValueError, match="'typo', which is not a command-line argument"): + build_parser_from_function(do_x, groups=(Group("typo"),)) + + +class TestEagerGroupSpecValidation: + """Spec-*shape* rules hard-fail at decoration time (class definition), not on first command use. + + The decorator runs the spec-shape checks before deferring the parser build, so a misshapen group + raises while the class body executes instead of surfacing as a swallowed runtime error the first + time the command runs. These checks read the ``Group`` specs only -- never the signature and never + the type hints -- so forward-referenced annotations still decorate. + + Whether a member *exists* is deliberately not among them: an ArgumentBlock's arguments are its + dataclass fields, which only resolving its hint would reveal, so that check runs at parser build + (see :class:`TestBuildTimeMemberValidation`). + """ def test_param_in_two_argument_groups_fails_at_decoration(self) -> None: def do_x(self, a: str = "v") -> None: ... @@ -1503,19 +1620,13 @@ def do_x(self, a: str = "v", b: str = "w") -> None: ... mutually_exclusive_groups=(Group("a", "b", title="U"),), )(do_x) - def test_subcommand_group_typo_fails_at_decoration(self) -> None: - def team_add(self, a: str = "v") -> None: ... - - with pytest.raises(ValueError, match="nonexistent parameter 'typo'"): - with_annotated(subcommand_to="team", groups=(Group("typo"),))(team_add) - def test_eager_validation_does_not_resolve_type_hints(self) -> None: - # The group error fires even though the annotation can never resolve: the checks read - # parameter names only, so the unresolvable hint is not touched. + # The shape error fires even though the annotation can never resolve: the checks read the + # Group specs only, so the unresolvable hint is not touched. def do_x(self, a: "NoSuchType" = None) -> None: ... # noqa: F821 - with pytest.raises(ValueError, match="nonexistent parameter 'typo'"): - with_annotated(groups=(Group("typo"),))(do_x) + with pytest.raises(ValueError, match="only valid in mutually_exclusive_groups"): + with_annotated(groups=(Group("a", required=True),))(do_x) def test_unresolvable_hints_with_valid_groups_decorate(self) -> None: # Valid specs + an unresolvable annotation must decorate without raising; type resolution