Raise ValueError in attach_subcommand() when the subcommand already exists. (#1645)

Author: kmvanbrunt

cmd2/argparse_utils.py

@@ -516,7 +516,7 @@ def _SubParsersAction_remove_parser(  # noqa: N802
     :raises ValueError: if the subcommand doesn't exist
     """
     if name not in self._name_parser_map:
-        raise ValueError(f"Subcommand '{name}' not found")
+        raise ValueError(f"Subcommand '{name}' does not exist")
 
     subparser = self._name_parser_map[name]
 
@@ -684,12 +684,12 @@ def update_prog(self, prog: str) -> None:
         # add_parser() will have the correct prog value.
         subparsers_action._prog_prefix = self._build_subparsers_prog_prefix(positionals)
 
-        # subparsers_action.choices includes aliases. Since primary names are inserted first,
-        # we skip already updated parsers to ensure primary names are used in 'prog'.
+        # subparsers_action._name_parser_map includes aliases. Since primary names are inserted
+        # first, we skip already updated parsers to ensure primary names are used in 'prog'.
         updated_parsers: set[Cmd2ArgumentParser] = set()
 
         # Set the prog value for each subcommand's parser
-        for subcmd_name, subcmd_parser in subparsers_action.choices.items():
+        for subcmd_name, subcmd_parser in subparsers_action._name_parser_map.items():
             if subcmd_parser in updated_parsers:
                 continue
 
@@ -707,9 +707,9 @@ def find_parser(self, subcommand_path: Iterable[str]) -> "Cmd2ArgumentParser":
         parser = self
         for name in subcommand_path:
             subparsers_action = parser.get_subparsers_action()
-            if name not in subparsers_action.choices:
-                raise ValueError(f"Subcommand '{name}' not found in '{parser.prog}'")
-            parser = subparsers_action.choices[name]
+            if name not in subparsers_action._name_parser_map:
+                raise ValueError(f"Subcommand '{name}' does not exist for '{parser.prog}'")
+            parser = subparsers_action._name_parser_map[name]
         return parser
 
     def attach_subcommand(
@@ -729,7 +729,8 @@ def attach_subcommand(
         :raises TypeError: if subcommand_parser is not an instance of the following or their subclasses:
                            1. Cmd2ArgumentParser
                            2. The parser_class configured for the target subcommand group
-        :raises ValueError: if the command path is invalid or doesn't support subcommands
+        :raises ValueError: if the command path is invalid, doesn't support subcommands, or the
+                            subcommand already exists
         """
         if not isinstance(subcommand_parser, Cmd2ArgumentParser):
             raise TypeError(
@@ -751,6 +752,12 @@ def attach_subcommand(
                 f"Received: '{type(subcommand_parser).__name__}'."
             )
 
+        # Do not overwrite existing subcommands or aliases
+        all_names = (subcommand, *add_parser_kwargs.get("aliases", ()))
+        for name in all_names:
+            if name in subparsers_action._name_parser_map:
+                raise ValueError(f"Subcommand '{name}' already exists for '{target_parser.prog}'")
+
         # Use add_parser to register the subcommand name and any aliases
         placeholder_parser = subparsers_action.add_parser(subcommand, **add_parser_kwargs)
 
@@ -783,7 +790,7 @@ def detach_subcommand(self, subcommand_path: Iterable[str], subcommand: str) ->
                 subparsers_action.remove_parser(subcommand),  # type: ignore[attr-defined]
             )
         except ValueError:
-            raise ValueError(f"Subcommand '{subcommand}' not found in '{target_parser.prog}'") from None
+            raise ValueError(f"Subcommand '{subcommand}' does not exist for '{target_parser.prog}'") from None
 
     def error(self, message: str) -> NoReturn:
         """Override that applies custom formatting to the error message."""

cmd2/cmd2.py

@@ -1174,7 +1174,7 @@ def get_root_parser_and_subcmd_path(self, command: str) -> tuple[Cmd2ArgumentPar
         # Search for the base command function and verify it has an argparser defined
         command_func = self.get_command_func(root_command)
         if command_func is None:
-            raise ValueError(f"Root command '{root_command}' not found")
+            raise ValueError(f"Root command '{root_command}' does not exist")
 
         root_parser = self.command_parsers.get(command_func)
         if root_parser is None:
@@ -1199,7 +1199,8 @@ def attach_subcommand(
         :raises TypeError: if subcommand_parser is not an instance of the following or their subclasses:
                            1. Cmd2ArgumentParser
                            2. The parser_class configured for the target subcommand group
-        :raises ValueError: if the command path is invalid or doesn't support subcommands
+        :raises ValueError: if the command path is invalid, doesn't support subcommands, or the
+                            subcommand already exists
         """
         root_parser, subcommand_path = self.get_root_parser_and_subcmd_path(command)
         root_parser.attach_subcommand(subcommand_path, subcommand, subcommand_parser, **add_parser_kwargs)

cmd2/decorators.py

@@ -360,9 +360,9 @@ def as_subcommand_to(
     :param subcommand: Subcommand name
     :param parser: instance of Cmd2ArgumentParser or a callable that returns a Cmd2ArgumentParser for this subcommand
     :param help: Help message for this subcommand which displays in the list of subcommands of the command we are adding to.
-                 This is passed as the help argument to subparsers.add_parser().
-    :param aliases: Alternative names for this subcommand. This is passed as the alias argument to
-                    subparsers.add_parser().
+                 If not None, this is passed as the 'help' argument to subparsers.add_parser().
+    :param aliases: Alternative names for this subcommand. If a non-empty sequence is provided, it is passed
+                    as the 'aliases' argument to subparsers.add_parser().
     :param add_parser_kwargs: other registration-specific kwargs for add_parser()
                               (e.g. deprecated [Python 3.13+])
     :return: a decorator which configures the target function to be a subcommand handler

tests/test_argparse_utils.py

@@ -453,20 +453,26 @@ def test_subcommand_attachment_errors() -> None:
     root_parser.add_subparsers()
 
     # Verify ValueError when path is invalid (find_parser() fails)
-    with pytest.raises(ValueError, match="Subcommand 'nonexistent' not found"):
+    with pytest.raises(ValueError, match="Subcommand 'nonexistent' does not exist for 'root'"):
         root_parser.attach_subcommand(["nonexistent"], "anything", child_parser)
-    with pytest.raises(ValueError, match="Subcommand 'nonexistent' not found"):
+    with pytest.raises(ValueError, match="Subcommand 'nonexistent' does not exist for 'root'"):
         root_parser.detach_subcommand(["nonexistent"], "anything")
 
     # Verify ValueError when path is valid but subcommand name is wrong
-    with pytest.raises(ValueError, match="Subcommand 'fake' not found in 'root'"):
+    with pytest.raises(ValueError, match="Subcommand 'fake' does not exist for 'root'"):
         root_parser.detach_subcommand([], "fake")
 
     # Verify TypeError when attaching a non-Cmd2ArgumentParser type
     ap_parser = argparse.ArgumentParser(prog="non-cmd2-parser")
     with pytest.raises(TypeError, match=r"must be an instance of 'Cmd2ArgumentParser' \(or a subclass\)"):
         root_parser.attach_subcommand([], "sub", ap_parser)  # type: ignore[arg-type]
 
+    # Verify ValueError when subcommand name already exists
+    sub_parser = Cmd2ArgumentParser(prog="sub")
+    root_parser.attach_subcommand([], "sub", sub_parser)
+    with pytest.raises(ValueError, match="Subcommand 'sub' already exists for 'root'"):
+        root_parser.attach_subcommand([], "sub", sub_parser)
+
 
 def test_subcommand_attachment_parser_class_override() -> None:
     class MyParser(Cmd2ArgumentParser):

tests/test_cmd2.py

@@ -4596,6 +4596,13 @@ class SubcmdErrorApp(cmd2.Cmd):
         def __init__(self) -> None:
             super().__init__()
 
+        test_parser = cmd2.Cmd2ArgumentParser()
+        test_parser.add_subparsers(required=True)
+
+        @cmd2.with_argparser(test_parser)
+        def do_test(self, _statement: cmd2.Statement) -> None:
+            pass
+
         def do_no_argparse(self, _statement: cmd2.Statement) -> None:
             pass
 
@@ -4606,9 +4613,14 @@ def do_no_argparse(self, _statement: cmd2.Statement) -> None:
         app.attach_subcommand("", "sub", cmd2.Cmd2ArgumentParser())
 
     # Test non-existent command
-    with pytest.raises(ValueError, match="Root command 'fake' not found"):
+    with pytest.raises(ValueError, match="Root command 'fake' does not exist"):
         app.attach_subcommand("fake", "sub", cmd2.Cmd2ArgumentParser())
 
     # Test command that doesn't use argparse
     with pytest.raises(ValueError, match="Command 'no_argparse' does not use argparse"):
         app.attach_subcommand("no_argparse", "sub", cmd2.Cmd2ArgumentParser())
+
+    # Test duplicate subcommand
+    app.attach_subcommand("test", "sub", cmd2.Cmd2ArgumentParser())
+    with pytest.raises(ValueError, match="Subcommand 'sub' already exists for 'test'"):
+        app.attach_subcommand("test", "sub", cmd2.Cmd2ArgumentParser())