chore: address comment and update test

Author: KelvinChung2000

cmd2/annotated.py

@@ -318,7 +318,6 @@ def _convert(value: str) -> Any:
         raise argparse.ArgumentTypeError(f"invalid choice: {value!r} (choose from {valid})")
 
     _convert.__name__ = "literal"
-    _convert._cmd2_strict_choice_converter = True  # type: ignore[attr-defined]
     return _convert
 
 
@@ -341,7 +340,6 @@ def _convert(value: str) -> enum.Enum:
 
     _convert.__name__ = enum_class.__name__
     _convert._cmd2_enum_class = enum_class  # type: ignore[attr-defined]
-    _convert._cmd2_strict_choice_converter = True  # type: ignore[attr-defined]
     return _convert
 
 
@@ -596,15 +594,23 @@ def _resolve_type(
     if is_kw_only and not has_default:
         kwargs["required"] = True
 
-    # An optional positional scalar (``T | None`` without a default) takes 0-or-1 tokens.
-    if is_optional and is_positional and "nargs" not in kwargs and not kwargs.get("is_collection"):
+    # An optional positional scalar takes 0-or-1 tokens. This covers both ``T | None``
+    # (no default) and a positional given an explicit default; without ``nargs='?'``
+    # argparse would still require the latter, contradicting its default value.
+    if (is_optional or has_default) and is_positional and "nargs" not in kwargs and not kwargs.get("is_collection"):
         kwargs["nargs"] = "?"
 
+    if is_positional and (is_optional or has_default) and isinstance(kwargs.get("nargs"), int):
+        raise TypeError(
+            f"A fixed-arity positional (nargs={kwargs['nargs']}) cannot be optional; argparse always "
+            f"requires it. Drop the default or '| None', make it an option (give it a default without "
+            f"Argument()), or use a variable-arity type such as tuple[T, ...]."
+        )
+
+    # A user-supplied completer/choices_provider drives completion, so drop the inferred
+    # static ``choices`` list.
     if kwargs.get("choices_provider") or kwargs.get("completer"):
         kwargs.pop("choices", None)
-        converter = kwargs.get("type")
-        if getattr(converter, "_cmd2_strict_choice_converter", False):
-            kwargs.pop("type", None)
 
     return base_type, kwargs
 

docs/features/annotated.md

@@ -108,6 +108,11 @@ Unsupported patterns raise `TypeError`, including:
 - `Annotated[T, Argument(nargs=N)]` where `N` is `'*'`, `'+'`, or an integer `>= 1` and `T` is not a
   collection type. `nargs` values that produce a list of values need a collection annotation such as
   `list[T]` or `tuple[T, ...]`.
+- an optional fixed-arity positional, such as `Annotated[tuple[int, int], Argument()] = (1, 2)`,
+  `Annotated[tuple[int, int] | None, Argument()]`, or any positional `Argument(nargs=N)` with a
+  default or `| None`. argparse cannot make a fixed-arity positional optional (there is no `nargs`
+  for "absent or exactly `N` tokens"), so use a variable-arity type like `tuple[T, ...]`, drop the
+  default, or make it an option (give it a default without `Argument()`).
 - `Annotated[tuple[T, T], Argument(nargs=N)]` where `N` differs from the number of elements declared
   by the tuple. The tuple type already pins `nargs`; user metadata cannot change it.
 
@@ -155,9 +160,11 @@ When an `Option(action=...)` uses an argparse action that does not accept `type=
 inferred `type` converter before calling `add_argument()`. This matches argparse behavior and avoids
 parser-construction errors such as combining `action='count'` with `type=int`.
 
-When a user-supplied `choices_provider` or `completer` overrides an inferred `Enum` or `Literal`,
-the restrictive type converter is also dropped so the user-supplied values are not rejected at parse
-time. The `Path` converter is permissive and is preserved when a custom completer is provided.
+When a user-supplied `choices_provider` or `completer` is given for an inferred `Enum` or `Literal`,
+the inferred static `choices` list is dropped so completion is driven by the provider or completer.
+The inferred `type` converter is preserved, so parsed values still coerce to the declared type
+(`Literal[1, 2]` yields an `int`, an `Enum` yields its member) and values outside the type are
+rejected at parse time.
 
 ## Decorator options
 

tests/test_annotated.py

@@ -106,10 +106,10 @@ def _func_literal_option(self, mode: Literal["fast", "slow"] = "fast") -> None:
 def _func_literal_int(self, level: Literal[1, 2, 3]) -> None: ...
 def _func_optional(self, name: str | None = None) -> None: ...
 def _func_optional_positional(self, val: Annotated[int | None, Argument()]) -> None: ...
+def _func_positional_with_default(self, arg: Annotated[str, Argument()] = "foo") -> None: ...
 def _func_optional_plain(self, val: int | None) -> None: ...
 def _func_optional_list(self, vals: list[int] | None) -> None: ...
 def _func_optional_tuple_ellipsis(self, vals: tuple[int, ...] | None) -> None: ...
-def _func_optional_explicit_nargs(self, vals: Annotated[tuple[int, ...] | None, Argument(nargs=2)]) -> None: ...
 def _func_list(self, files: list[str]) -> None: ...
 def _func_list_default(self, items: list[str] | None = None) -> None: ...
 def _func_set(self, tags: set[str]) -> None: ...
@@ -243,22 +243,26 @@ class TestBuildParser:
             pytest.param(
                 _func_optional_positional, {"option_strings": [], "nargs": "?", "type": int}, id="optional_positional"
             ),
+            pytest.param(
+                _func_positional_with_default,
+                {"option_strings": [], "nargs": "?", "default": "foo"},
+                id="positional_with_default",
+            ),
             pytest.param(_func_optional_plain, {"option_strings": [], "nargs": "?", "type": int}, id="optional_plain"),
             pytest.param(_func_optional_list, {"option_strings": [], "nargs": "*", "type": int}, id="optional_list"),
             pytest.param(
                 _func_optional_tuple_ellipsis,
                 {"option_strings": [], "nargs": "*", "type": int},
                 id="optional_tuple_ellipsis",
             ),
-            pytest.param(
-                _func_optional_explicit_nargs,
-                {"option_strings": [], "nargs": 2, "type": int},
-                id="optional_explicit_nargs_overrides",
-            ),
             # --- Options ---
             pytest.param(_func_int_option, {"option_strings": ["--count"], "type": int, "default": 1}, id="int_option"),
             pytest.param(_func_float_option, {"option_strings": ["--rate"], "type": float, "default": 1.0}, id="float_option"),
-            pytest.param(_func_bool_false, {"option_strings": ["--verbose", "--no-verbose"]}, id="bool_optional_action"),
+            pytest.param(
+                _func_bool_false,
+                {"option_strings": ["--verbose", "--no-verbose"], "default": False},
+                id="bool_optional_action",
+            ),
             pytest.param(
                 _func_bool_true,
                 {"option_strings": ["--debug", "--no-debug"], "default": True},
@@ -375,6 +379,28 @@ def test_annotated_action_append(self) -> None:
         assert isinstance(action, argparse._AppendAction)
         assert action.option_strings == ["--tag"]
 
+    def test_positional_with_default_is_optional(self) -> None:
+        """A positional with a default takes 0-or-1 tokens and falls back to the default when absent."""
+        parser = build_parser_from_function(_func_positional_with_default)
+        assert parser.parse_args([]).arg == "foo"
+        assert parser.parse_args(["bar"]).arg == "bar"
+
+    def test_str_default_on_int_option_coerced_at_parse(self) -> None:
+        """The decorator stores the default literally ('1', see ``default_not_coerced``); at parse
+        time argparse applies ``type=int`` to the string default, so an absent ``--count`` yields int 1.
+        """
+        parser = build_parser_from_function(_func_default_type_mismatch)
+        assert parser.parse_args([]).count == 1
+        assert parser.parse_args(["--count", "5"]).count == 5
+
+    def test_typing_optional_parses_end_to_end(self) -> None:
+        """typing.Optional[int] yields None when absent and coerces to int when provided."""
+        parser = build_parser_from_function(_func_typing_optional)
+        assert parser.parse_args([]).count is None
+        parsed = parser.parse_args(["--count", "5"]).count
+        assert parsed == 5
+        assert isinstance(parsed, int)
+
     @pytest.mark.parametrize(
         "func",
         [
@@ -455,25 +481,64 @@ class TestTypeInferenceBuildParser:
     def test_choices_provider_overrides_inferred_enum_choices(self) -> None:
         action = _get_param_action(_func_choices_provider_on_enum)
         assert action.choices is None
-        assert action.get_choices_provider() is not None  # type: ignore[attr-defined]
+        assert action.get_choices_provider() is _provider  # type: ignore[attr-defined]
         assert action.get_completer() is None  # type: ignore[attr-defined]
 
-    def test_choices_provider_strips_strict_enum_converter(self) -> None:
-        """User-supplied choices_provider on Enum drops the restrictive enum converter."""
+    def test_choices_provider_keeps_enum_coercion(self) -> None:
+        """A choices_provider on an Enum keeps the converter so values still coerce to the member."""
         action = _get_param_action(_func_choices_provider_on_enum)
-        assert action.type is None
+        assert action.type is not None
+        assert action.type("red") is _Color.red
 
-    def test_choices_provider_strips_strict_literal_converter(self) -> None:
-        """User-supplied choices_provider on Literal drops the restrictive literal converter."""
+    def test_choices_provider_keeps_literal_coercion(self) -> None:
+        """A choices_provider on a Literal keeps the converter (coercion) but drops the static choices."""
 
         def func(
             self,
-            mode: Annotated[Literal["fast", "slow"], Argument(choices_provider=_provider)],
+            level: Annotated[Literal[1, 2], Argument(choices_provider=_provider)],
         ) -> None: ...
 
         action = _get_param_action(func)
-        assert action.type is None
         assert action.choices is None
+        assert action.type is not None
+        assert action.type("1") == 1
+
+    def test_choices_provider_enum_coerces_at_parse(self) -> None:
+        """End-to-end: an Enum with a choices_provider still parses to the enum member, not a str."""
+        parser = build_parser_from_function(_func_choices_provider_on_enum)
+        assert parser.parse_args(["red"]).color is _Color.red
+
+    def test_choices_provider_literal_int_coerces_at_parse(self) -> None:
+        """End-to-end: a Literal[int] with a choices_provider parses to int, not a str."""
+
+        def func(
+            self,
+            level: Annotated[Literal[1, 2], Argument(choices_provider=_provider)],
+        ) -> None: ...
+
+        parser = build_parser_from_function(func)
+        parsed = parser.parse_args(["1"]).level
+        assert parsed == 1
+        assert isinstance(parsed, int)
+
+    def test_bare_enum_literal_coerce_at_parse(self) -> None:
+        """Bare Enum/Literal positionals and options coerce to the declared type at parse time.
+
+        Uses identity / isinstance (not ``==``) so a stripped converter returning a raw ``str``
+        cannot hide behind StrEnum/IntEnum equality.
+        """
+        assert build_parser_from_function(_func_literal).parse_args(["fast"]).mode == "fast"
+        assert build_parser_from_function(_func_literal_option).parse_args(["--mode", "slow"]).mode == "slow"
+
+        level = build_parser_from_function(_func_literal_int).parse_args(["2"]).level
+        assert level == 2
+        assert isinstance(level, int)
+
+        assert build_parser_from_function(_func_enum).parse_args(["red"]).color is _Color.red
+        assert build_parser_from_function(_func_enum_option).parse_args(["--color", "red"]).color is _Color.red
+        # Non-StrEnum cases: identity defeats the StrEnum/IntEnum ``==`` masking property.
+        assert build_parser_from_function(_func_int_enum).parse_args(["1"]).color is _IntColor.red
+        assert build_parser_from_function(_func_plain_enum).parse_args(["red"]).color is _PlainColor.RED
 
     def test_completer_keeps_path_converter(self) -> None:
         """User-supplied completer on Path preserves the (non-restrictive) Path converter."""
@@ -875,6 +940,28 @@ def test_nargs_overrides_fixed_arity_raises(self, annotation) -> None:
         with pytest.raises(TypeError, match=r"conflicts with the fixed arity"):
             _resolve_annotation(annotation)
 
+    @pytest.mark.parametrize(
+        ("annotation", "resolve_kwargs"),
+        [
+            pytest.param(
+                Annotated[tuple[int, int], Argument()],
+                {"has_default": True, "default": (1, 2)},
+                id="fixed_tuple_with_default",
+            ),
+            pytest.param(Annotated[tuple[int, int] | None, Argument()], {}, id="fixed_tuple_optional"),
+            pytest.param(
+                Annotated[tuple[int, ...], Argument(nargs=2)],
+                {"has_default": True, "default": (1, 2)},
+                id="explicit_nargs_with_default",
+            ),
+            pytest.param(Annotated[tuple[int, ...] | None, Argument(nargs=2)], {}, id="explicit_nargs_optional"),
+        ],
+    )
+    def test_optional_fixed_arity_positional_raises(self, annotation, resolve_kwargs) -> None:
+        """argparse cannot make a fixed-arity positional optional, so the combination is rejected."""
+        with pytest.raises(TypeError, match=r"fixed-arity positional"):
+            _resolve_annotation(annotation, **resolve_kwargs)
+
     @pytest.mark.parametrize(
         "annotation",
         [
@@ -1202,7 +1289,7 @@ def do_add(self, a: int, b: int = 0) -> None:
     def do_paint(
         self,
         item: str,
-        color: Annotated[_Color, Option("--color", "-c", help_text="Color")] = _Color.blue,
+        color: Annotated[_Color, Option("--color", "-c", help_text="Color to use")] = _Color.blue,
         verbose: bool = False,
     ) -> None:
         msg = f"Painting {item} {color.value}"
@@ -1263,7 +1350,7 @@ def test_help_shows_arguments(self, runtime_app) -> None:
     def test_help_shows_option_help(self, runtime_app) -> None:
         out, _ = run_cmd(runtime_app, "help paint")
         help_text = "\n".join(out)
-        assert "Color" in help_text or "color" in help_text
+        assert "Color to use" in help_text
 
 
 class TestRuntimeCompletion:
@@ -1569,16 +1656,16 @@ def test_subcommand_executes(self, subcmd_app, command, expected) -> None:
         assert out == expected
 
     @pytest.mark.parametrize(
-        "command",
+        ("command", "expected_error"),
         [
-            pytest.param("manage", id="missing_subcmd"),
-            pytest.param("manage delete", id="invalid_subcmd"),
-            pytest.param("manage member", id="missing_nested_subcmd"),
+            pytest.param("manage", "the following arguments are required: SUBCOMMAND", id="missing_subcmd"),
+            pytest.param("manage delete", "invalid choice: 'delete'", id="invalid_subcmd"),
+            pytest.param("manage member", "the following arguments are required: SUBCOMMAND", id="missing_nested_subcmd"),
         ],
     )
-    def test_subcommand_errors(self, subcmd_app, command) -> None:
+    def test_subcommand_errors(self, subcmd_app, command, expected_error) -> None:
         _out, err = run_cmd(subcmd_app, command)
-        assert any("error" in line.lower() or "usage" in line.lower() or "invalid" in line.lower() for line in err)
+        assert any(expected_error in line for line in err), f"expected {expected_error!r} in {err}"
 
     def test_subcommand_help(self, subcmd_app) -> None:
         out, _err = run_cmd(subcmd_app, "help manage")