chore: more clean up

Author: KelvinChung2000

cmd2/annotated.py

@@ -13,8 +13,10 @@
 Basic usage -- parameters without defaults become positional arguments,
 parameters with defaults become ``--option`` flags.  Keyword-only
 parameters (after ``*``) always become options; without a default they
-are required.  The parameter name ``dest`` is reserved and cannot be
-used::
+are required.  Underscores in parameter names are auto-converted to
+dashes in the generated flag (``dry_run`` -> ``--dry-run``); pass
+explicit names via ``Option("--my_flag")`` to opt out.  The parameter
+name ``dest`` is reserved and cannot be used::
 
     class MyApp(cmd2.Cmd):
         @cmd2.with_annotated
@@ -76,7 +78,13 @@ def do_paint(
 Note: ``Path`` and ``Enum`` annotations with ``@with_annotated`` also get
 automatic tab completion via generated parser metadata.
 If a user-supplied ``choices_provider`` or ``completer`` is set on an argument,
-it always takes priority over the type-inferred completion.
+it always takes priority over the type-inferred completion.  For ``Enum`` and
+``Literal``, the restrictive type converter is also stripped so user-supplied
+values are not rejected at parse time.  The ``Path`` converter is permissive
+and is preserved when a custom completer is provided.
+
+The parameter name ``cmd2_handler`` is reserved for base commands declared with
+``with_annotated(base_command=True)`` and may not be used elsewhere.
 """
 
 import argparse
@@ -100,7 +108,6 @@ def do_paint(
 
 from . import constants
 from .argparse_utils import Cmd2ArgumentParser, SubcommandSpec
-from .cmd2 import Cmd
 from .completion import CompletionItem
 from .decorators import _parse_positionals
 from .exceptions import Cmd2ArgparseError
@@ -166,7 +173,8 @@ class Option(_BaseArgMetadata):
     """Metadata for an optional/flag argument in an ``Annotated`` type hint.
 
     Positional ``*names`` are the flag strings (e.g. ``"--color"``, ``"-c"``).
-    When omitted, the decorator auto-generates ``--param_name``.
+    When omitted, the decorator auto-generates ``--param-name`` (underscores
+    in the parameter name are converted to dashes).
 
     Example::
 
@@ -193,10 +201,10 @@ def __init__(
     def to_kwargs(self) -> dict[str, Any]:
         """Return non-None fields as an argparse kwargs dict."""
         kwargs = super().to_kwargs()
-        if self.action:
+        if self.action is not None:
             kwargs["action"] = self.action
         if self.required:
-            kwargs["required"] = self.required
+            kwargs["required"] = True
         return kwargs
 
 
@@ -265,6 +273,7 @@ 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
 
 
@@ -287,6 +296,7 @@ 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
 
 
@@ -324,6 +334,8 @@ def _resolve(_tp: Any, _args: tuple[Any, ...], **_ctx: Any) -> dict[str, Any]:
 
 def _resolve_path(_tp: Any, _args: tuple[Any, ...], **_ctx: Any) -> dict[str, Any]:
     """Resolve Path and add completer."""
+    from .cmd2 import Cmd
+
     return {"type": Path, "completer": Cmd.path_complete}
 
 
@@ -339,8 +351,8 @@ def _resolve_bool(
     if not is_positional:
         action_str = getattr(metadata, "action", None) if metadata else None
         if action_str:
-            return {"action": action_str, "is_bool_flag": True}
-        return {"action": argparse.BooleanOptionalAction, "is_bool_flag": True}
+            return {"action": action_str}
+        return {"action": argparse.BooleanOptionalAction}
     return {"type": _parse_bool, "choices": list(_BOOL_CHOICES)}
 
 
@@ -504,6 +516,9 @@ def _resolve_type(
 
     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
 
@@ -572,8 +587,8 @@ def _resolve_annotation(
     has_default: bool = False,
     default: Any = None,
     is_kw_only: bool = False,
-) -> tuple[dict[str, Any], ArgMetadata, bool, bool]:
-    """Decompose a type annotation into ``(type_kwargs, metadata, is_positional, is_bool_flag)``.
+) -> tuple[dict[str, Any], ArgMetadata, bool]:
+    """Decompose a type annotation into ``(type_kwargs, metadata, is_positional)``.
 
     Peels ``Annotated`` then ``Optional``.  The only supported way to combine
     ``Annotated`` with ``Optional`` is ``Annotated[T | None, meta]``.
@@ -585,7 +600,6 @@ def _resolve_annotation(
         not isinstance(metadata, Option) and not has_default and not is_optional and not is_kw_only
     )
 
-    # 4. Resolve type and finalize argparse kwargs
     tp, type_kwargs = _resolve_type(
         tp,
         is_positional=is_positional,
@@ -595,12 +609,10 @@ def _resolve_annotation(
         is_kw_only=is_kw_only,
     )
 
-    # Strip internal keys not meant for argparse
-    is_bool_flag = type_kwargs.pop("is_bool_flag", False)
     type_kwargs.pop("is_collection", None)
     type_kwargs.pop("base_type", None)
 
-    return type_kwargs, metadata, is_positional, is_bool_flag
+    return type_kwargs, metadata, is_positional
 
 
 # Parameter names that conflict with argparse internals and cannot be used
@@ -619,9 +631,7 @@ def _validate_base_command_params(
     skip_params: frozenset[str] | None = None,
 ) -> None:
     """Validate a ``base_command=True`` function has ``cmd2_handler`` and no positional args."""
-    sig = inspect.signature(func)
-
-    if "cmd2_handler" not in sig.parameters:
+    if "cmd2_handler" not in inspect.signature(func).parameters:
         raise TypeError(f"with_annotated(base_command=True) requires a 'cmd2_handler' parameter in {func.__qualname__}")
 
     if skip_params is None:
@@ -689,7 +699,7 @@ def _resolve_parameters(
         default = param.default if has_default else None
         is_kw_only = param.kind == inspect.Parameter.KEYWORD_ONLY
 
-        kwargs, metadata, positional, _is_bool_flag = _resolve_annotation(
+        kwargs, metadata, positional = _resolve_annotation(
             annotation,
             has_default=has_default,
             default=default,
@@ -699,7 +709,9 @@ def _resolve_parameters(
         if positional:
             flags: list[str] = []
         else:
-            flags = list(metadata.names) if isinstance(metadata, Option) and metadata.names else [f"--{name}"]
+            flags = (
+                list(metadata.names) if isinstance(metadata, Option) and metadata.names else [f"--{name.replace('_', '-')}"]
+            )
             kwargs["dest"] = name
 
         resolved.append((name, metadata, positional, flags, kwargs))
@@ -987,6 +999,11 @@ def decorator(fn: Callable[..., Any]) -> Callable[..., Any]:
             if unknown_param.kind is inspect.Parameter.POSITIONAL_ONLY:
                 raise TypeError("Parameter _unknown must be keyword-compatible when with_unknown_args=True")
 
+        if not base_command and "cmd2_handler" in inspect.signature(fn).parameters:
+            raise TypeError(
+                f"Parameter 'cmd2_handler' in {fn.__qualname__} is only valid when with_annotated(base_command=True) is used."
+            )
+
         if subcommand_to is not None:
             handler, subcmd_name, subcmd_parser_builder = build_subcommand_handler(
                 fn,

tests/test_annotated.py

@@ -254,7 +254,7 @@ class TestBuildParser:
             pytest.param(_func_kw_only, {"option_strings": ["--name"], "required": True}, id="kw_only_required"),
             pytest.param(_func_kw_only_with_default, {"option_strings": ["--name"], "default": "world"}, id="kw_only_default"),
             # --- Underscore in flag names ---
-            pytest.param(_func_underscore_option, {"option_strings": ["--my_param"], "default": "x"}, id="underscore_flag"),
+            pytest.param(_func_underscore_option, {"option_strings": ["--my-param"], "default": "x"}, id="underscore_flag"),
             # --- Default type preservation ---
             pytest.param(
                 _func_default_type_mismatch, {"option_strings": ["--count"], "default": "1"}, id="default_not_coerced"
@@ -365,6 +365,28 @@ def test_choices_provider_overrides_inferred_enum_choices(self) -> None:
         assert action.get_choices_provider() is not None  # 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."""
+        action = _get_param_action(_func_choices_provider_on_enum)
+        assert action.type is None
+
+    def test_choices_provider_strips_strict_literal_converter(self) -> None:
+        """User-supplied choices_provider on Literal drops the restrictive literal converter."""
+
+        def func(
+            self,
+            mode: Annotated[Literal["fast", "slow"], Argument(choices_provider=_provider)],
+        ) -> None: ...
+
+        action = _get_param_action(func)
+        assert action.type is None
+        assert action.choices is None
+
+    def test_completer_keeps_path_converter(self) -> None:
+        """User-supplied completer on Path preserves the (non-restrictive) Path converter."""
+        action = _get_param_action(_func_completer_on_path)
+        assert action.type is Path
+
     def test_completer_overrides_inferred_path_completion(self) -> None:
         action = _get_param_action(_func_completer_on_path)
         assert action.get_choices_provider() is None  # type: ignore[attr-defined]
@@ -558,7 +580,7 @@ def test_apply_mutex_group_targets_rejects_cross_group_members(self) -> None:
 
 
 # ---------------------------------------------------------------------------
-# _resolve_annotation: positional vs option classification + bool flag
+# _resolve_annotation: positional vs option classification
 # ---------------------------------------------------------------------------
 
 _ARG_META = Argument(help_text="Name")
@@ -567,39 +589,38 @@ def test_apply_mutex_group_targets_rejects_cross_group_members(self) -> None:
 
 class TestResolveAnnotation:
     @pytest.mark.parametrize(
-        ("annotation", "has_default", "expected_positional", "expected_bool_flag"),
+        ("annotation", "has_default", "expected_positional"),
         [
-            pytest.param(str, False, True, False, id="plain_str"),
-            pytest.param(str | None, False, False, False, id="optional_str"),
-            pytest.param(Annotated[str, _ARG_META], False, True, False, id="annotated_argument"),
-            pytest.param(Annotated[str, _OPT_META], False, False, False, id="annotated_option"),
-            pytest.param(Annotated[str, "some doc"], False, True, False, id="annotated_no_meta"),
-            pytest.param(str, True, False, False, id="has_default"),
-            pytest.param(bool, True, False, True, id="bool_flag"),
+            pytest.param(str, False, True, id="plain_str"),
+            pytest.param(str | None, False, False, id="optional_str"),
+            pytest.param(Annotated[str, _ARG_META], False, True, id="annotated_argument"),
+            pytest.param(Annotated[str, _OPT_META], False, False, id="annotated_option"),
+            pytest.param(Annotated[str, "some doc"], False, True, id="annotated_no_meta"),
+            pytest.param(str, True, False, id="has_default"),
+            pytest.param(bool, True, False, id="bool_flag"),
         ],
     )
-    def test_classification(self, annotation, has_default, expected_positional, expected_bool_flag) -> None:
-        _kwargs, _meta, positional, is_bool_flag = _resolve_annotation(annotation, has_default=has_default)
+    def test_classification(self, annotation, has_default, expected_positional) -> None:
+        _kwargs, _meta, positional = _resolve_annotation(annotation, has_default=has_default)
         assert positional is expected_positional
-        assert is_bool_flag is expected_bool_flag
 
     def test_optional_wrapping_annotated_with_none_inside(self) -> None:
         """Optional[Annotated[T | None, meta]] is allowed (inner type contains None)."""
         ann = Annotated[str | None, _OPT_META] | None
-        _kwargs, meta, positional, _bf = _resolve_annotation(ann)
+        _kwargs, meta, positional = _resolve_annotation(ann)
         assert meta is _OPT_META
         assert positional is False
 
     def test_typing_union_optional(self) -> None:
         ns: dict = {}
         exec("import typing; t = typing.Union[str, None]", ns)
-        _kwargs, _meta, positional, _bool_flag = _resolve_annotation(ns["t"])
+        _kwargs, _meta, positional = _resolve_annotation(ns["t"])
         assert positional is False
 
     def test_annotated_multiple_metadata_picks_first(self) -> None:
         meta1 = Argument(help_text="first")
         meta2 = Option("--x", help_text="second")
-        kwargs, meta, _, _ = _resolve_annotation(Annotated[str, meta1, meta2])
+        kwargs, meta, _ = _resolve_annotation(Annotated[str, meta1, meta2])
         assert meta is meta1
         assert kwargs.get("help") == "first"
 
@@ -639,7 +660,7 @@ def test_nested_collection_raises(self, annotation) -> None:
         ],
     )
     def test_unsupported_collection_no_nargs(self, annotation) -> None:
-        kwargs, _, _, _ = _resolve_annotation(annotation)
+        kwargs, _, _ = _resolve_annotation(annotation)
         assert "nargs" not in kwargs
         assert "action" not in kwargs
 
@@ -1085,7 +1106,9 @@ def do_transfer(
 
 @pytest.fixture
 def app() -> _IntegrationApp:
-    return _IntegrationApp()
+    app = _IntegrationApp()
+    app.stdout = cmd2.utils.StdSim(app.stdout)
+    return app
 
 
 @pytest.fixture
@@ -1147,12 +1170,29 @@ def test_ns_provider(self, app) -> None:
         assert app.ns_calls == 1
 
     def test_cmd2_prefixed_param_is_preserved(self, app) -> None:
-        out, _err = run_cmd(app, "prefixed --cmd2_mode 5")
+        out, _err = run_cmd(app, "prefixed --cmd2-mode 5")
         assert out == ["cmd2_mode=5"]
 
     def test_kwargs_passthrough(self, app) -> None:
         app.do_greet("Alice", keyword_arg="kwarg_value")
 
+    def test_direct_call_with_positional_only(self, app) -> None:
+        """Calling do_* directly with a single statement string parses normally."""
+        app.do_greet("Alice")
+        assert app.stdout.getvalue().splitlines()[-1] == "Hello Alice"
+
+    def test_direct_call_with_options(self, app) -> None:
+        """Direct call with a full statement string including options."""
+        app.do_greet("Alice --count 2 --loud")
+        out = app.stdout.getvalue().splitlines()
+        assert out[-2:] == ["HELLO ALICE", "HELLO ALICE"]
+
+    def test_direct_call_kwargs_override_parsed(self, app) -> None:
+        """Explicit kwargs on a direct call override parsed values."""
+        app.do_greet("Alice", count=3)
+        out = app.stdout.getvalue().splitlines()
+        assert out[-3:] == ["Hello Alice", "Hello Alice", "Hello Alice"]
+
     def test_bare_call_decorator(self) -> None:
         """@with_annotated() with empty parens works same as @with_annotated."""
 
@@ -1176,7 +1216,7 @@ def test_missing_parser_raises(self, app) -> None:
 
 class TestGroupedParserIntegration:
     def test_grouped_command_executes(self, grouped_app) -> None:
-        out, _err = run_cmd(grouped_app, "transfer --local build.tar.gz --dry_run")
+        out, _err = run_cmd(grouped_app, "transfer --local build.tar.gz --dry-run")
         assert out == ["Transfer build.tar.gz in dry-run mode"]
 
     def test_grouped_command_mutex_error(self, grouped_app) -> None:
@@ -1189,7 +1229,7 @@ def test_grouped_command_help_lists_flags(self, grouped_app) -> None:
         assert "--local" in help_text
         assert "--remote" in help_text
         assert "--force" in help_text
-        assert "--dry_run" in help_text
+        assert "--dry-run" in help_text
 
 
 # ---------------------------------------------------------------------------
@@ -1293,6 +1333,14 @@ def test_base_command_missing_handler_raises(self) -> None:
             def do_bad(self, verbose: bool = False) -> None:
                 pass
 
+    def test_cmd2_handler_without_base_command_raises(self) -> None:
+        """A 'cmd2_handler' parameter is only valid when base_command=True."""
+        with pytest.raises(TypeError, match="base_command=True"):
+
+            @cmd2.with_annotated
+            def do_bad(self, cmd2_handler, name: str = "") -> None:
+                pass
+
     @pytest.mark.parametrize(
         "kwargs",
         [