From fd655f4c3fdb07997364b7cf33a4f7b046448891 Mon Sep 17 00:00:00 2001 From: Kevin Van Brunt Date: Wed, 22 Apr 2026 14:26:47 -0400 Subject: [PATCH 1/3] Raise ValueError in attach_subcommand() when the subcommand already exists. --- cmd2/argparse_utils.py | 9 +++++++-- cmd2/cmd2.py | 3 ++- tests/test_argparse_utils.py | 6 ++++++ tests/test_cmd2.py | 12 ++++++++++++ 4 files changed, 27 insertions(+), 3 deletions(-) diff --git a/cmd2/argparse_utils.py b/cmd2/argparse_utils.py index c01086401..6f0599932 100644 --- a/cmd2/argparse_utils.py +++ b/cmd2/argparse_utils.py @@ -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( @@ -752,7 +753,11 @@ def attach_subcommand( ) # Use add_parser to register the subcommand name and any aliases - placeholder_parser = subparsers_action.add_parser(subcommand, **add_parser_kwargs) + try: + placeholder_parser = subparsers_action.add_parser(subcommand, **add_parser_kwargs) + except ArgumentError as ex: + # The subcommand already exists + raise ValueError(str(ex)) from None # To ensure accurate usage strings, recursively update 'prog' values # within the injected parser to match its new location in the command hierarchy. diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 0d80dd007..000968b67 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -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) diff --git a/tests/test_argparse_utils.py b/tests/test_argparse_utils.py index 0bdf932f6..7deae8aba 100644 --- a/tests/test_argparse_utils.py +++ b/tests/test_argparse_utils.py @@ -467,6 +467,12 @@ def test_subcommand_attachment_errors() -> None: 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="conflicting subparser: sub"): + root_parser.attach_subcommand([], "sub", sub_parser) + def test_subcommand_attachment_parser_class_override() -> None: class MyParser(Cmd2ArgumentParser): diff --git a/tests/test_cmd2.py b/tests/test_cmd2.py index 39aad2d27..671e9ff7f 100644 --- a/tests/test_cmd2.py +++ b/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 @@ -4612,3 +4619,8 @@ def do_no_argparse(self, _statement: cmd2.Statement) -> None: # 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="conflicting subparser: sub"): + app.attach_subcommand("test", "sub", cmd2.Cmd2ArgumentParser()) From 1dc704241c9a2d15fb4f67f71e89d89f572ba808 Mon Sep 17 00:00:00 2001 From: Kevin Van Brunt Date: Wed, 22 Apr 2026 16:27:57 -0400 Subject: [PATCH 2/3] Fixed inconsistent behavior between Python versions by always checking for duplicate subcommands. --- cmd2/argparse_utils.py | 28 +++++++++++++++------------- cmd2/cmd2.py | 2 +- tests/test_argparse_utils.py | 8 ++++---- tests/test_cmd2.py | 4 ++-- 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/cmd2/argparse_utils.py b/cmd2/argparse_utils.py index 6f0599932..b71ba7275 100644 --- a/cmd2/argparse_utils.py +++ b/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( @@ -752,12 +752,14 @@ 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 - try: - placeholder_parser = subparsers_action.add_parser(subcommand, **add_parser_kwargs) - except ArgumentError as ex: - # The subcommand already exists - raise ValueError(str(ex)) from None + placeholder_parser = subparsers_action.add_parser(subcommand, **add_parser_kwargs) # To ensure accurate usage strings, recursively update 'prog' values # within the injected parser to match its new location in the command hierarchy. @@ -788,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.""" diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 000968b67..325985bbd 100644 --- a/cmd2/cmd2.py +++ b/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: diff --git a/tests/test_argparse_utils.py b/tests/test_argparse_utils.py index 7deae8aba..3be2263f4 100644 --- a/tests/test_argparse_utils.py +++ b/tests/test_argparse_utils.py @@ -453,13 +453,13 @@ 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 @@ -470,7 +470,7 @@ def test_subcommand_attachment_errors() -> None: # Verify ValueError when subcommand name already exists sub_parser = Cmd2ArgumentParser(prog="sub") root_parser.attach_subcommand([], "sub", sub_parser) - with pytest.raises(ValueError, match="conflicting subparser: sub"): + with pytest.raises(ValueError, match="Subcommand 'sub' already exists for 'root'"): root_parser.attach_subcommand([], "sub", sub_parser) diff --git a/tests/test_cmd2.py b/tests/test_cmd2.py index 671e9ff7f..330ab97b7 100644 --- a/tests/test_cmd2.py +++ b/tests/test_cmd2.py @@ -4613,7 +4613,7 @@ 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 @@ -4622,5 +4622,5 @@ def do_no_argparse(self, _statement: cmd2.Statement) -> None: # Test duplicate subcommand app.attach_subcommand("test", "sub", cmd2.Cmd2ArgumentParser()) - with pytest.raises(ValueError, match="conflicting subparser: sub"): + with pytest.raises(ValueError, match="Subcommand 'sub' already exists for 'test'"): app.attach_subcommand("test", "sub", cmd2.Cmd2ArgumentParser()) From abdd76c4bad0ca2c88d97f09dc84626452e74095 Mon Sep 17 00:00:00 2001 From: Kevin Van Brunt Date: Wed, 22 Apr 2026 17:21:50 -0400 Subject: [PATCH 3/3] Updated comment. --- cmd2/decorators.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd2/decorators.py b/cmd2/decorators.py index 204161b44..d743b7b0b 100644 --- a/cmd2/decorators.py +++ b/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