From 475b28c768046aa0235843aaaa3fe7476f74666c Mon Sep 17 00:00:00 2001 From: Kevin Van Brunt Date: Thu, 21 Nov 2024 21:11:51 -0500 Subject: [PATCH 1/7] Fixed issue where argument parsers for overridden commands were not being created. --- CHANGELOG.md | 4 + cmd2/cmd2.py | 237 +++++++++++------- cmd2/decorators.py | 4 +- pyproject.toml | 1 + tests/test_argparse.py | 4 +- tests/test_cmd2.py | 35 +++ tests/transcripts/from_cmdloop.txt | 2 +- .../test_commandset/test_commandset.py | 58 ++++- 8 files changed, 253 insertions(+), 92 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 57efc7b0..850bae2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +## 2.5.7 (TBD) +* Bug Fixes + * Fixed issue where argument parsers for overridden commands were not being created. + ## 2.5.6 (November 14, 2024) * Bug Fixes * Fixed type hint for `with_default_category` decorator which caused type checkers to mistype diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index e9878b23..94a59f7f 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -218,6 +218,81 @@ def __init__(self) -> None: ClassArgParseBuilder = classmethod +class _CommandParsers: + """ + Create and store all command method argument parsers for a given Cmd instance. + + Parser creation and retrieval are accomplished through the get() method. + """ + + def __init__(self, cmd: 'Cmd') -> None: + self._cmd = cmd + + # Keyed by the fully qualified method names. This is more reliable than + # the methods themselves, since wrapping a method will change its address. + self._parsers: Dict[str, argparse.ArgumentParser] = {} + + @staticmethod + def _fully_qualified_name(command_method: CommandFunc) -> str: + """Return the fully qualified name of a method or None if a method wasn't passed in.""" + try: + return f"{command_method.__module__}.{command_method.__qualname__}" + except AttributeError: + return "" + + def __contains__(self, command_method: CommandFunc) -> bool: + """ + Return whether a given method's parser is in self. + + If the parser does not yet exist, it will be created if applicable. + This is basically for checking if a method is argarse-based. + """ + parser = self.get(command_method) + return bool(parser) + + def get(self, command_method: CommandFunc) -> Optional[argparse.ArgumentParser]: + """ + Return a given method's parser or None if the method is not argparse-based. + + If the parser does not yet exist, it will be created. + """ + full_method_name = self._fully_qualified_name(command_method) + if not full_method_name: + return None + + if full_method_name not in self._parsers: + if not command_method.__name__.startswith(COMMAND_FUNC_PREFIX): + return None + command = command_method.__name__[len(COMMAND_FUNC_PREFIX) :] + + parser_builder = getattr(command_method, constants.CMD_ATTR_ARGPARSER, None) + parent = self._cmd.find_commandset_for_command(command) or self._cmd + parser = self._cmd._build_parser(parent, parser_builder) + if parser is None: + return None + + # argparser defaults the program name to sys.argv[0], but we want it to be the name of our command + from .decorators import ( + _set_parser_prog, + ) + + _set_parser_prog(parser, command) + + # If the description has not been set, then use the method docstring if one exists + if parser.description is None and hasattr(command_method, '__wrapped__') and command_method.__wrapped__.__doc__: + parser.description = strip_doc_annotations(command_method.__wrapped__.__doc__) + + self._parsers[full_method_name] = parser + + return self._parsers.get(full_method_name) + + def remove(self, command_method: CommandFunc) -> None: + """Remove a given method's parser if it exists.""" + full_method_name = self._fully_qualified_name(command_method) + if full_method_name in self._parsers: + del self._parsers[full_method_name] + + class Cmd(cmd.Cmd): """An easy but powerful framework for writing line-oriented command interpreters. @@ -537,11 +612,7 @@ def __init__( self.matches_sorted = False # Command parsers for this Cmd instance. - self._command_parsers: Dict[str, argparse.ArgumentParser] = {} - - # Locates the command parser template or factory and creates an instance-specific parser - for command in self.get_all_commands(): - self._register_command_parser(command, self.cmd_func(command)) # type: ignore[arg-type] + self._command_parsers = _CommandParsers(self) # Add functions decorated to be subcommands self._register_subcommands(self) @@ -657,11 +728,11 @@ def register_command_set(self, cmdset: CommandSet) -> None: installed_attributes = [] try: - for method_name, method in methods: - command = method_name[len(COMMAND_FUNC_PREFIX) :] + for cmd_func_name, command_method in methods: + command = cmd_func_name[len(COMMAND_FUNC_PREFIX) :] - self._install_command_function(command, method, type(cmdset).__name__) - installed_attributes.append(method_name) + self._install_command_function(cmd_func_name, command_method, type(cmdset).__name__) + installed_attributes.append(cmd_func_name) completer_func_name = COMPLETER_FUNC_PREFIX + command cmd_completer = getattr(cmdset, completer_func_name, None) @@ -677,8 +748,8 @@ def register_command_set(self, cmdset: CommandSet) -> None: self._cmd_to_command_sets[command] = cmdset - if default_category and not hasattr(method, constants.CMD_ATTR_HELP_CATEGORY): - utils.categorize(method, default_category) + if default_category and not hasattr(command_method, constants.CMD_ATTR_HELP_CATEGORY): + utils.categorize(command_method, default_category) self._installed_command_sets.add(cmdset) @@ -718,33 +789,31 @@ def _build_parser( parser = copy.deepcopy(parser_builder) return parser - def _register_command_parser(self, command: str, command_method: Callable[..., Any]) -> None: - if command not in self._command_parsers: - parser_builder = getattr(command_method, constants.CMD_ATTR_ARGPARSER, None) - parent = self.find_commandset_for_command(command) or self - parser = self._build_parser(parent, parser_builder) - if parser is None: - return - - # argparser defaults the program name to sys.argv[0], but we want it to be the name of our command - from .decorators import ( - _set_parser_prog, - ) + def _install_command_function(self, command_func_name: str, command_method: CommandFunc, context: str = '') -> None: + """ + Install a new command function into the CLI. - _set_parser_prog(parser, command) + :param command_func_name: name of command function to add + This points to the command method and may differ from the method's + name if it's being used as a synonym. (e.g. do_exit = do_quit) + :param command_method: the actual command method which runs when the command function is called + :param context: optional info to provide in error message. (e.g. class this function belongs to) + :raises CommandSetRegistrationError: if the command function fails to install + """ - # If the description has not been set, then use the method docstring if one exists - if parser.description is None and hasattr(command_method, '__wrapped__') and command_method.__wrapped__.__doc__: - parser.description = strip_doc_annotations(command_method.__wrapped__.__doc__) + # command_func_name must begin with COMMAND_FUNC_PREFIX to be identified as a command by cmd2. + if not command_func_name.startswith(COMMAND_FUNC_PREFIX): + raise CommandSetRegistrationError(f"{command_func_name} does not begin with '{COMMAND_FUNC_PREFIX}'") - self._command_parsers[command] = parser + # command_method must start with COMMAND_FUNC_PREFIX for use in self._command_parsers. + if not command_method.__name__.startswith(COMMAND_FUNC_PREFIX): + raise CommandSetRegistrationError(f"{command_method.__name__} does not begin with '{COMMAND_FUNC_PREFIX}'") - def _install_command_function(self, command: str, command_wrapper: Callable[..., Any], context: str = '') -> None: - cmd_func_name = COMMAND_FUNC_PREFIX + command + command = command_func_name[len(COMMAND_FUNC_PREFIX) :] # Make sure command function doesn't share name with existing attribute - if hasattr(self, cmd_func_name): - raise CommandSetRegistrationError(f'Attribute already exists: {cmd_func_name} ({context})') + if hasattr(self, command_func_name): + raise CommandSetRegistrationError(f'Attribute already exists: {command_func_name} ({context})') # Check if command has an invalid name valid, errmsg = self.statement_parser.is_valid_command(command) @@ -761,9 +830,7 @@ def _install_command_function(self, command: str, command_wrapper: Callable[..., self.pwarning(f"Deleting macro '{command}' because it shares its name with a new command") del self.macros[command] - self._register_command_parser(command, command_wrapper) - - setattr(self, cmd_func_name, command_wrapper) + setattr(self, command_func_name, command_method) def _install_completer_function(self, cmd_name: str, cmd_completer: CompleterFunc) -> None: completer_func_name = COMPLETER_FUNC_PREFIX + cmd_name @@ -790,47 +857,52 @@ def unregister_command_set(self, cmdset: CommandSet) -> None: cmdset.on_unregister() self._unregister_subcommands(cmdset) - methods: List[Tuple[str, Callable[[Any], Any]]] = inspect.getmembers( + methods: List[Tuple[str, Callable[..., Any]]] = inspect.getmembers( cmdset, predicate=lambda meth: isinstance(meth, Callable) # type: ignore[arg-type] and hasattr(meth, '__name__') and meth.__name__.startswith(COMMAND_FUNC_PREFIX), ) - for method in methods: - cmd_name = method[0][len(COMMAND_FUNC_PREFIX) :] + for cmd_func_name, command_method in methods: + command = cmd_func_name[len(COMMAND_FUNC_PREFIX) :] # Enable the command before uninstalling it to make sure we remove both # the real functions and the ones used by the DisabledCommand object. - if cmd_name in self.disabled_commands: - self.enable_command(cmd_name) + if command in self.disabled_commands: + self.enable_command(command) - if cmd_name in self._cmd_to_command_sets: - del self._cmd_to_command_sets[cmd_name] + if command in self._cmd_to_command_sets: + del self._cmd_to_command_sets[command] - delattr(self, COMMAND_FUNC_PREFIX + cmd_name) - if cmd_name in self._command_parsers: - del self._command_parsers[cmd_name] + # A command synonym does not own the parser. + if cmd_func_name == command_method.__name__: + self._command_parsers.remove(command_method) - if hasattr(self, COMPLETER_FUNC_PREFIX + cmd_name): - delattr(self, COMPLETER_FUNC_PREFIX + cmd_name) - if hasattr(self, HELP_FUNC_PREFIX + cmd_name): - delattr(self, HELP_FUNC_PREFIX + cmd_name) + if hasattr(self, COMPLETER_FUNC_PREFIX + command): + delattr(self, COMPLETER_FUNC_PREFIX + command) + if hasattr(self, HELP_FUNC_PREFIX + command): + delattr(self, HELP_FUNC_PREFIX + command) + + delattr(self, cmd_func_name) cmdset.on_unregistered() self._installed_command_sets.remove(cmdset) def _check_uninstallable(self, cmdset: CommandSet) -> None: - methods: List[Tuple[str, Callable[[Any], Any]]] = inspect.getmembers( + methods: List[Tuple[str, Callable[..., Any]]] = inspect.getmembers( cmdset, predicate=lambda meth: isinstance(meth, Callable) # type: ignore[arg-type] and hasattr(meth, '__name__') and meth.__name__.startswith(COMMAND_FUNC_PREFIX), ) - for method in methods: - command_name = method[0][len(COMMAND_FUNC_PREFIX) :] - command_parser = self._command_parsers.get(command_name, None) + for cmd_func_name, command_method in methods: + # Do nothing if this is a command synonym since it does not own the parser. + if cmd_func_name != command_method.__name__: + continue + + command_parser = self._command_parsers.get(command_method) def check_parser_uninstallable(parser: argparse.ArgumentParser) -> None: for action in parser._actions: @@ -889,7 +961,7 @@ def _register_subcommands(self, cmdset: Union[CommandSet, 'Cmd']) -> None: raise CommandSetRegistrationError( f"Could not find command '{command_name}' needed by subcommand: {str(method)}" ) - command_parser = self._command_parsers.get(command_name, None) + command_parser = self._command_parsers.get(command_func) if command_parser is None: raise CommandSetRegistrationError( f"Could not find argparser for command '{command_name}' needed by subcommand: {str(method)}" @@ -991,7 +1063,7 @@ def _unregister_subcommands(self, cmdset: Union[CommandSet, 'Cmd']) -> None: raise CommandSetRegistrationError( f"Could not find command '{command_name}' needed by subcommand: {str(method)}" ) - command_parser = self._command_parsers.get(command_name, None) + command_parser = self._command_parsers.get(command_func) if command_parser is None: # pragma: no cover # This really shouldn't be possible since _register_subcommands would prevent this from happening # but keeping in case it does for some strange reason @@ -2098,12 +2170,12 @@ def _perform_completion( else: # There's no completer function, next see if the command uses argparse func = self.cmd_func(command) - argparser = self._command_parsers.get(command, None) + argparser = None if func is None else self._command_parsers.get(func) if func is not None and argparser is not None: # Get arguments for complete() preserve_quotes = getattr(func, constants.CMD_ATTR_PRESERVE_QUOTES) - cmd_set = self._cmd_to_command_sets[command] if command in self._cmd_to_command_sets else None + cmd_set = self.find_commandset_for_command(command) # Create the argparse completer completer_type = self._determine_ap_completer_type(argparser) @@ -3044,19 +3116,9 @@ def cmd_func(self, command: str) -> Optional[CommandFunc]: helpfunc now contains a reference to the ``do_help`` method """ - func_name = self._cmd_func_name(command) - if func_name: - return cast(Optional[CommandFunc], getattr(self, func_name)) - return None - - def _cmd_func_name(self, command: str) -> str: - """Get the method name associated with a given command. - - :param command: command to look up method name which implements it - :return: method name which implements the given command - """ - target = constants.COMMAND_FUNC_PREFIX + command - return target if callable(getattr(self, target, None)) else '' + func_name = constants.COMMAND_FUNC_PREFIX + command + func = getattr(self, func_name, None) + return cast(CommandFunc, func) if callable(func) else None def onecmd(self, statement: Union[Statement, str], *, add_to_history: bool = True) -> bool: """This executes the actual do_* method for a command. @@ -3404,7 +3466,7 @@ def _cmdloop(self) -> None: alias_description = "Manage aliases\n" "\n" "An alias is a command that enables replacement of a word by another string." alias_epilog = "See also:\n" " macro" alias_parser = argparse_custom.DEFAULT_ARGUMENT_PARSER(description=alias_description, epilog=alias_epilog) - alias_parser.add_subparsers(dest='subcommand', metavar='SUBCOMMAND', required=True) + alias_parser.add_subparsers(metavar='SUBCOMMAND', required=True) # Preserve quotes since we are passing strings to other commands @with_argparser(alias_parser, preserve_quotes=True) @@ -3572,7 +3634,7 @@ def _alias_list(self, args: argparse.Namespace) -> None: macro_description = "Manage macros\n" "\n" "A macro is similar to an alias, but it can contain argument placeholders." macro_epilog = "See also:\n" " alias" macro_parser = argparse_custom.DEFAULT_ARGUMENT_PARSER(description=macro_description, epilog=macro_epilog) - macro_parser.add_subparsers(dest='subcommand', metavar='SUBCOMMAND', required=True) + macro_parser.add_subparsers(metavar='SUBCOMMAND', required=True) # Preserve quotes since we are passing strings to other commands @with_argparser(macro_parser, preserve_quotes=True) @@ -3820,9 +3882,7 @@ def complete_help_subcommands( return [] # Check if this command uses argparse - func = self.cmd_func(command) - argparser = self._command_parsers.get(command, None) - if func is None or argparser is None: + if (func := self.cmd_func(command)) is None or (argparser := self._command_parsers.get(func)) is None: return [] completer = argparse_completer.DEFAULT_AP_COMPLETER(argparser, self) @@ -3857,7 +3917,7 @@ def do_help(self, args: argparse.Namespace) -> None: # Getting help for a specific command func = self.cmd_func(args.command) help_func = getattr(self, constants.HELP_FUNC_PREFIX + args.command, None) - argparser = self._command_parsers.get(args.command, None) + argparser = None if func is None else self._command_parsers.get(func) # If the command function uses argparse, then use argparse's help if func is not None and argparser is not None: @@ -3979,28 +4039,29 @@ def _help_menu(self, verbose: bool = False) -> None: def _build_command_info(self) -> Tuple[Dict[str, List[str]], List[str], List[str], List[str]]: # Get a sorted list of help topics help_topics = sorted(self.get_help_topics(), key=self.default_sort_key) + # Get a sorted list of visible command names visible_commands = sorted(self.get_visible_commands(), key=self.default_sort_key) cmds_doc: List[str] = [] cmds_undoc: List[str] = [] cmds_cats: Dict[str, List[str]] = {} for command in visible_commands: - func = self.cmd_func(command) + func = cast(CommandFunc, self.cmd_func(command)) has_help_func = False + has_parser = func in self._command_parsers if command in help_topics: # Prevent the command from showing as both a command and help topic in the output help_topics.remove(command) # Non-argparse commands can have help_functions for their documentation - if command not in self._command_parsers: - has_help_func = True + has_help_func = not has_parser if hasattr(func, constants.CMD_ATTR_HELP_CATEGORY): category: str = getattr(func, constants.CMD_ATTR_HELP_CATEGORY) cmds_cats.setdefault(category, []) cmds_cats[category].append(command) - elif func.__doc__ or has_help_func: + elif func.__doc__ or has_help_func or has_parser: cmds_doc.append(command) else: cmds_undoc.append(command) @@ -4035,11 +4096,17 @@ def _print_topics(self, header: str, cmds: List[str], verbose: bool) -> None: # Try to get the documentation string for each command topics = self.get_help_topics() for command in cmds: - cmd_func = self.cmd_func(command) + if (cmd_func := self.cmd_func(command)) is None: + continue + doc: Optional[str] + # If this is an argparse command, use its description. + if (cmd_parser := self._command_parsers.get(cmd_func)) is not None: + doc = cmd_parser.description + # Non-argparse commands can have help_functions for their documentation - if command not in self._command_parsers and command in topics: + elif command in topics: help_func = getattr(self, constants.HELP_FUNC_PREFIX + command) result = io.StringIO() @@ -5436,12 +5503,13 @@ def enable_command(self, command: str) -> None: if command not in self.disabled_commands: return + cmd_func_name = constants.COMMAND_FUNC_PREFIX + command help_func_name = constants.HELP_FUNC_PREFIX + command completer_func_name = constants.COMPLETER_FUNC_PREFIX + command # Restore the command function to its original value dc = self.disabled_commands[command] - setattr(self, self._cmd_func_name(command), dc.command_function) + setattr(self, cmd_func_name, dc.command_function) # Restore the help function to its original value if dc.help_function is None: @@ -5489,6 +5557,7 @@ def disable_command(self, command: str, message_to_print: str) -> None: if command_function is None: raise AttributeError(f"'{command}' does not refer to a command") + cmd_func_name = constants.COMMAND_FUNC_PREFIX + command help_func_name = constants.HELP_FUNC_PREFIX + command completer_func_name = constants.COMPLETER_FUNC_PREFIX + command @@ -5503,7 +5572,7 @@ def disable_command(self, command: str, message_to_print: str) -> None: new_func = functools.partial( self._report_disabled_command_usage, message_to_print=message_to_print.replace(constants.COMMAND_NAME, command) ) - setattr(self, self._cmd_func_name(command), new_func) + setattr(self, cmd_func_name, new_func) setattr(self, help_func_name, new_func) # Set the completer to a function that returns a blank list diff --git a/cmd2/decorators.py b/cmd2/decorators.py index 0dac33cc..2e73d6b0 100644 --- a/cmd2/decorators.py +++ b/cmd2/decorators.py @@ -346,7 +346,9 @@ def cmd_wrapper(*args: Any, **kwargs: Dict[str, Any]) -> Optional[bool]: statement, parsed_arglist = cmd2_app.statement_parser.get_command_arg_list( command_name, statement_arg, preserve_quotes ) - arg_parser = cmd2_app._command_parsers.get(command_name, None) + + # Pass cmd_wrapper instead of func, since it contains the parser info. + arg_parser = cmd2_app._command_parsers.get(cmd_wrapper) if arg_parser is None: # This shouldn't be possible to reach raise ValueError(f'No argument parser found for {command_name}') # pragma: no cover diff --git a/pyproject.toml b/pyproject.toml index 7cf142ad..8d9192eb 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -328,6 +328,7 @@ dev-dependencies = [ "pytest", "pytest-cov", "pytest-mock", + "ruff", "sphinx", "sphinx-autobuild", "sphinx-rtd-theme", diff --git a/tests/test_argparse.py b/tests/test_argparse.py index cf1eda92..c2731d37 100644 --- a/tests/test_argparse.py +++ b/tests/test_argparse.py @@ -14,7 +14,6 @@ import cmd2 from .conftest import ( - find_subcommand, run_cmd, ) @@ -402,8 +401,7 @@ def test_add_another_subcommand(subcommand_app): This tests makes sure _set_parser_prog() sets _prog_prefix on every _SubParsersAction so that all future calls to add_parser() write the correct prog value to the parser being added. """ - base_parser = subcommand_app._command_parsers.get('base') - find_subcommand(subcommand_app._command_parsers.get('base'), []) + base_parser = subcommand_app._command_parsers.get(subcommand_app.do_base) for sub_action in base_parser._actions: if isinstance(sub_action, argparse._SubParsersAction): new_parser = sub_action.add_parser('new_sub', help='stuff') diff --git a/tests/test_cmd2.py b/tests/test_cmd2.py index 374ba10a..f91969a1 100755 --- a/tests/test_cmd2.py +++ b/tests/test_cmd2.py @@ -1210,6 +1210,13 @@ def do_multiline_docstr(self, arg): """ pass + parser_cmd_parser = cmd2.Cmd2ArgumentParser(description="This is the description.") + + @cmd2.with_argparser(parser_cmd_parser) + def do_parser_cmd(self, args): + """This is the docstring.""" + pass + @pytest.fixture def help_app(): @@ -1249,6 +1256,11 @@ def test_help_multiline_docstring(help_app): assert help_app.last_result is True +def test_help_verbose_uses_parser_description(help_app: HelpApp): + out, err = run_cmd(help_app, 'help --verbose') + verify_help_text(help_app, out, verbose_strings=[help_app.parser_cmd_parser.description]) + + class HelpCategoriesApp(cmd2.Cmd): """Class for testing custom help_* methods which override docstring help.""" @@ -3016,3 +3028,26 @@ def test_columnize_too_wide(outsim_app): expected = "\n".join(str_list) + "\n" assert outsim_app.stdout.getvalue() == expected + + +def test_command_parser_retrieval(outsim_app: cmd2.Cmd): + # Pass something that isn't a method + not_a_method = "just a string" + assert outsim_app._command_parsers.get(not_a_method) is None + + # Pass a non-command method + assert outsim_app._command_parsers.get(outsim_app.__init__) is None + + +def test_command_synonym_parser(): + # Make sure a command synonym returns the same parser as what it aliases + class SynonymApp(cmd2.cmd2.Cmd): + do_synonym = cmd2.cmd2.Cmd.do_help + + app = SynonymApp() + + synonym_parser = app._command_parsers.get(app.do_synonym) + help_parser = app._command_parsers.get(app.do_help) + + assert synonym_parser is not None + assert synonym_parser is help_parser diff --git a/tests/transcripts/from_cmdloop.txt b/tests/transcripts/from_cmdloop.txt index a615d243..f1c68d81 100644 --- a/tests/transcripts/from_cmdloop.txt +++ b/tests/transcripts/from_cmdloop.txt @@ -2,7 +2,7 @@ # so you can see where they are. (Cmd) help say -Usage: say [-h] [-p] [-s] [-r REPEAT]/ */ +Usage: speak [-h] [-p] [-s] [-r REPEAT]/ */ Repeats what you tell me to./ */ diff --git a/tests_isolated/test_commandset/test_commandset.py b/tests_isolated/test_commandset/test_commandset.py index 36384abd..3d4e5b2d 100644 --- a/tests_isolated/test_commandset/test_commandset.py +++ b/tests_isolated/test_commandset/test_commandset.py @@ -149,6 +149,50 @@ def test_autoload_commands(command_sets_app): assert 'Command Set B' not in cmds_cats +def test_command_synonyms(): + """Test the use of command synonyms in CommandSets""" + + class SynonymCommandSet(cmd2.CommandSet): + def __init__(self, arg1): + super().__init__() + self._arg1 = arg1 + + @cmd2.with_argparser(cmd2.Cmd2ArgumentParser(description="Native Command")) + def do_builtin(self, _): + pass + + # Create a synonym to a command inside of this CommandSet + do_builtin_synonym = do_builtin + + # Create a synonym to a command outside of this CommandSet + do_help_synonym = cmd2.Cmd.do_help + + cs = SynonymCommandSet("foo") + app = WithCommandSets(command_sets=[cs]) + + # Make sure the synonyms have the same parser as what they alias + builtin_parser = app._command_parsers.get(app.do_builtin) + builtin_synonym_parser = app._command_parsers.get(app.do_builtin_synonym) + assert builtin_parser is not None + assert builtin_parser is builtin_synonym_parser + + help_parser = app._command_parsers.get(cmd2.Cmd.do_help) + help_synonym_parser = app._command_parsers.get(app.do_help_synonym) + assert help_parser is not None + assert help_parser is help_synonym_parser + + # Unregister the CommandSet and make sure built-in command and synonyms are gone + app.unregister_command_set(cs) + assert not hasattr(app, "do_builtin") + assert not hasattr(app, "do_builtin_synonym") + assert not hasattr(app, "do_help_synonym") + + # Make sure the help command still exists, has the same parser, and works. + assert help_parser is app._command_parsers.get(cmd2.Cmd.do_help) + out, err = run_cmd(app, 'help') + assert app.doc_header in out + + def test_custom_construct_commandsets(): command_set_b = CommandSetB('foo') @@ -291,7 +335,7 @@ def test_load_commandset_errors(command_sets_manual, capsys): cmd_set = CommandSetA() # create a conflicting command before installing CommandSet to verify rollback behavior - command_sets_manual._install_command_function('durian', cmd_set.do_durian) + command_sets_manual._install_command_function('do_durian', cmd_set.do_durian) with pytest.raises(CommandSetRegistrationError): command_sets_manual.register_command_set(cmd_set) @@ -316,13 +360,21 @@ def test_load_commandset_errors(command_sets_manual, capsys): assert "Deleting alias 'banana'" in err assert "Deleting macro 'apple'" in err + # verify command functions which don't start with "do_" raise an exception + with pytest.raises(CommandSetRegistrationError): + command_sets_manual._install_command_function('new_cmd', cmd_set.do_banana) + + # verify methods which don't start with "do_" raise an exception + with pytest.raises(CommandSetRegistrationError): + command_sets_manual._install_command_function('do_new_cmd', cmd_set.on_register) + # verify duplicate commands are detected with pytest.raises(CommandSetRegistrationError): - command_sets_manual._install_command_function('banana', cmd_set.do_banana) + command_sets_manual._install_command_function('do_banana', cmd_set.do_banana) # verify bad command names are detected with pytest.raises(CommandSetRegistrationError): - command_sets_manual._install_command_function('bad command', cmd_set.do_banana) + command_sets_manual._install_command_function('do_bad command', cmd_set.do_banana) # verify error conflict with existing completer function with pytest.raises(CommandSetRegistrationError): From 99ba8292bf6d97863b008897277365e975127039 Mon Sep 17 00:00:00 2001 From: Kevin Van Brunt Date: Thu, 21 Nov 2024 23:47:11 -0500 Subject: [PATCH 2/7] Fixed terminal check in Cmd.ppaged() to use correct output stream. --- CHANGELOG.md | 1 + cmd2/cmd2.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 850bae2b..6e721643 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ ## 2.5.7 (TBD) * Bug Fixes * Fixed issue where argument parsers for overridden commands were not being created. + * Fixed terminal check in `Cmd.ppaged()` to use correct output stream. ## 2.5.6 (November 14, 2024) * Bug Fixes diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 94a59f7f..78241afb 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -1419,7 +1419,7 @@ def ppaged(self, msg: Any, *, end: str = '\n', chop: bool = False, dest: Optiona # Don't try to use the pager when being run by a continuous integration system like Jenkins + pexpect. functional_terminal = False - if self.stdin.isatty() and self.stdout.isatty(): + if self.stdin.isatty() and dest.isatty(): if sys.platform.startswith('win') or os.environ.get('TERM') is not None: functional_terminal = True From 4b71f09addf4e969866b166f016e6bcdd0cac0ce Mon Sep 17 00:00:00 2001 From: Kevin Van Brunt Date: Fri, 22 Nov 2024 13:08:44 -0500 Subject: [PATCH 3/7] Updated unit test. --- cmd2/cmd2.py | 41 +++++++++---------- .../test_commandset/test_commandset.py | 24 ++++++----- 2 files changed, 33 insertions(+), 32 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 78241afb..3bb01698 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -875,7 +875,8 @@ def unregister_command_set(self, cmdset: CommandSet) -> None: if command in self._cmd_to_command_sets: del self._cmd_to_command_sets[command] - # A command synonym does not own the parser. + # Only remove the parser if this is the actual + # command since command synonyms don't own it. if cmd_func_name == command_method.__name__: self._command_parsers.remove(command_method) @@ -890,6 +891,18 @@ def unregister_command_set(self, cmdset: CommandSet) -> None: self._installed_command_sets.remove(cmdset) def _check_uninstallable(self, cmdset: CommandSet) -> None: + def check_parser_uninstallable(parser: argparse.ArgumentParser) -> None: + for action in parser._actions: + if isinstance(action, argparse._SubParsersAction): + for subparser in action.choices.values(): + attached_cmdset = getattr(subparser, constants.PARSER_ATTR_COMMANDSET, None) + if attached_cmdset is not None and attached_cmdset is not cmdset: + raise CommandSetRegistrationError( + 'Cannot uninstall CommandSet when another CommandSet depends on it' + ) + check_parser_uninstallable(subparser) + break + methods: List[Tuple[str, Callable[..., Any]]] = inspect.getmembers( cmdset, predicate=lambda meth: isinstance(meth, Callable) # type: ignore[arg-type] @@ -898,26 +911,12 @@ def _check_uninstallable(self, cmdset: CommandSet) -> None: ) for cmd_func_name, command_method in methods: - # Do nothing if this is a command synonym since it does not own the parser. - if cmd_func_name != command_method.__name__: - continue - - command_parser = self._command_parsers.get(command_method) - - def check_parser_uninstallable(parser: argparse.ArgumentParser) -> None: - for action in parser._actions: - if isinstance(action, argparse._SubParsersAction): - for subparser in action.choices.values(): - attached_cmdset = getattr(subparser, constants.PARSER_ATTR_COMMANDSET, None) - if attached_cmdset is not None and attached_cmdset is not cmdset: - raise CommandSetRegistrationError( - 'Cannot uninstall CommandSet when another CommandSet depends on it' - ) - check_parser_uninstallable(subparser) - break - - if command_parser is not None: - check_parser_uninstallable(command_parser) + # We only need to check if it's safe to remove the parser if this + # is the actual command since command synonyms don't own it. + if cmd_func_name == command_method.__name__: + command_parser = self._command_parsers.get(command_method) + if command_parser is not None: + check_parser_uninstallable(command_parser) def _register_subcommands(self, cmdset: Union[CommandSet, 'Cmd']) -> None: """ diff --git a/tests_isolated/test_commandset/test_commandset.py b/tests_isolated/test_commandset/test_commandset.py index 3d4e5b2d..89c982f3 100644 --- a/tests_isolated/test_commandset/test_commandset.py +++ b/tests_isolated/test_commandset/test_commandset.py @@ -164,8 +164,10 @@ def do_builtin(self, _): # Create a synonym to a command inside of this CommandSet do_builtin_synonym = do_builtin - # Create a synonym to a command outside of this CommandSet - do_help_synonym = cmd2.Cmd.do_help + # Create a synonym to a command outside of this CommandSet with subcommands. + # This will best test the synonym check in cmd2.Cmd._check_uninstallable() when + # we unresgister this CommandSet. + do_alias_synonym = cmd2.Cmd.do_alias cs = SynonymCommandSet("foo") app = WithCommandSets(command_sets=[cs]) @@ -176,21 +178,21 @@ def do_builtin(self, _): assert builtin_parser is not None assert builtin_parser is builtin_synonym_parser - help_parser = app._command_parsers.get(cmd2.Cmd.do_help) - help_synonym_parser = app._command_parsers.get(app.do_help_synonym) - assert help_parser is not None - assert help_parser is help_synonym_parser + alias_parser = app._command_parsers.get(cmd2.Cmd.do_alias) + alias_synonym_parser = app._command_parsers.get(app.do_alias_synonym) + assert alias_parser is not None + assert alias_parser is alias_synonym_parser # Unregister the CommandSet and make sure built-in command and synonyms are gone app.unregister_command_set(cs) assert not hasattr(app, "do_builtin") assert not hasattr(app, "do_builtin_synonym") - assert not hasattr(app, "do_help_synonym") + assert not hasattr(app, "do_alias_synonym") - # Make sure the help command still exists, has the same parser, and works. - assert help_parser is app._command_parsers.get(cmd2.Cmd.do_help) - out, err = run_cmd(app, 'help') - assert app.doc_header in out + # Make sure the alias command still exists, has the same parser, and works. + assert alias_parser is app._command_parsers.get(cmd2.Cmd.do_alias) + out, err = run_cmd(app, 'alias --help') + assert normalize(alias_parser.format_help())[0] in out def test_custom_construct_commandsets(): From d35aff2bede66c0a0a50fd3bb924f26aba840c05 Mon Sep 17 00:00:00 2001 From: Kevin Van Brunt Date: Fri, 22 Nov 2024 14:02:01 -0500 Subject: [PATCH 4/7] Fixed issue where Cmd.ppaged() was not writing to the passed in destination. --- CHANGELOG.md | 2 +- cmd2/cmd2.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e721643..39343359 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,7 @@ ## 2.5.7 (TBD) * Bug Fixes * Fixed issue where argument parsers for overridden commands were not being created. - * Fixed terminal check in `Cmd.ppaged()` to use correct output stream. + * Fixed issue where `Cmd.ppaged()` was not writing to the passed in destination. ## 2.5.6 (November 14, 2024) * Bug Fixes diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 3bb01698..70b0d8cd 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -1436,7 +1436,7 @@ def ppaged(self, msg: Any, *, end: str = '\n', chop: bool = False, dest: Optiona # Prevent KeyboardInterrupts while in the pager. The pager application will # still receive the SIGINT since it is in the same process group as us. with self.sigint_protection: - pipe_proc = subprocess.Popen(pager, shell=True, stdin=subprocess.PIPE) + pipe_proc = subprocess.Popen(pager, shell=True, stdin=subprocess.PIPE, stdout=dest) pipe_proc.communicate(msg_str.encode('utf-8', 'replace')) else: ansi.style_aware_write(dest, f'{msg_str}{end}') From 564adf4ec5f58b0d4877cc46d28c703dc0315e8c Mon Sep 17 00:00:00 2001 From: Kevin Van Brunt Date: Fri, 22 Nov 2024 14:15:43 -0500 Subject: [PATCH 5/7] Simplified ppaged(). --- cmd2/cmd2.py | 60 +++++++++++++++++++++------------------------- tests/test_cmd2.py | 14 ----------- 2 files changed, 27 insertions(+), 47 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 70b0d8cd..ea53d345 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -1403,49 +1403,43 @@ def ppaged(self, msg: Any, *, end: str = '\n', chop: bool = False, dest: Optiona WARNING: On Windows, the text always wraps regardless of what the chop argument is set to """ - # msg can be any type, so convert to string before checking if it's blank - msg_str = str(msg) dest = self.stdout if dest is None else dest - # Consider None to be no data to print - if msg is None or msg_str == '': - return - - try: - import subprocess + # Attempt to detect if we are not running within a fully functional terminal. + # Don't try to use the pager when being run by a continuous integration system like Jenkins + pexpect. + functional_terminal = False - # Attempt to detect if we are not running within a fully functional terminal. - # Don't try to use the pager when being run by a continuous integration system like Jenkins + pexpect. - functional_terminal = False + if self.stdin.isatty() and dest.isatty(): + if sys.platform.startswith('win') or os.environ.get('TERM') is not None: + functional_terminal = True - if self.stdin.isatty() and dest.isatty(): - if sys.platform.startswith('win') or os.environ.get('TERM') is not None: - functional_terminal = True + # Don't attempt to use a pager that can block if redirecting or running a script (either text or Python). + # Also only attempt to use a pager if actually running in a real fully functional terminal. + if functional_terminal and not self._redirecting and not self.in_pyscript() and not self.in_script(): + final_msg = f"{msg}{end}" + if ansi.allow_style == ansi.AllowStyle.NEVER: + final_msg = ansi.strip_style(final_msg) - # Don't attempt to use a pager that can block if redirecting or running a script (either text or Python) - # Also only attempt to use a pager if actually running in a real fully functional terminal - if functional_terminal and not self._redirecting and not self.in_pyscript() and not self.in_script(): - if ansi.allow_style == ansi.AllowStyle.NEVER: - msg_str = ansi.strip_style(msg_str) - msg_str += end - - pager = self.pager - if chop: - pager = self.pager_chop + pager = self.pager + if chop: + pager = self.pager_chop + try: # Prevent KeyboardInterrupts while in the pager. The pager application will # still receive the SIGINT since it is in the same process group as us. with self.sigint_protection: + import subprocess + pipe_proc = subprocess.Popen(pager, shell=True, stdin=subprocess.PIPE, stdout=dest) - pipe_proc.communicate(msg_str.encode('utf-8', 'replace')) - else: - ansi.style_aware_write(dest, f'{msg_str}{end}') - except BrokenPipeError: - # This occurs if a command's output is being piped to another process and that process closes before the - # command is finished. If you would like your application to print a warning message, then set the - # broken_pipe_warning attribute to the message you want printed.` - if self.broken_pipe_warning: - sys.stderr.write(self.broken_pipe_warning) + pipe_proc.communicate(final_msg.encode('utf-8', 'replace')) + except BrokenPipeError: + # This occurs if a command's output is being piped to another process and that process closes before the + # command is finished. If you would like your application to print a warning message, then set the + # broken_pipe_warning attribute to the message you want printed.` + if self.broken_pipe_warning: + sys.stderr.write(self.broken_pipe_warning) + else: + self.print_to(dest, msg, end=end, paged=False) # ----- Methods related to tab completion ----- diff --git a/tests/test_cmd2.py b/tests/test_cmd2.py index f91969a1..1df1ea2c 100755 --- a/tests/test_cmd2.py +++ b/tests/test_cmd2.py @@ -2480,20 +2480,6 @@ def test_ppaged(outsim_app): assert out == msg + end -def test_ppaged_blank(outsim_app): - msg = '' - outsim_app.ppaged(msg) - out = outsim_app.stdout.getvalue() - assert not out - - -def test_ppaged_none(outsim_app): - msg = None - outsim_app.ppaged(msg) - out = outsim_app.stdout.getvalue() - assert not out - - @with_ansi_style(ansi.AllowStyle.TERMINAL) def test_ppaged_strips_ansi_when_redirecting(outsim_app): msg = 'testing...' From c0991d8697a9d44352770b8c99aaa1e4288fa330 Mon Sep 17 00:00:00 2001 From: Kevin Van Brunt Date: Fri, 22 Nov 2024 22:05:23 -0500 Subject: [PATCH 6/7] Update change log for release. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39343359..ec86a319 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,4 @@ -## 2.5.7 (TBD) +## 2.5.7 (November 22, 2024) * Bug Fixes * Fixed issue where argument parsers for overridden commands were not being created. * Fixed issue where `Cmd.ppaged()` was not writing to the passed in destination. From abd8bdf7f6f4bd64eca88f1faa0d28df61210531 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 25 Nov 2024 12:18:35 -0500 Subject: [PATCH 7/7] Bump astral-sh/setup-uv from 3 to 4 (#1386) Bumps [astral-sh/setup-uv](https://github.com/astral-sh/setup-uv) from 3 to 4. - [Release notes](https://github.com/astral-sh/setup-uv/releases) - [Commits](https://github.com/astral-sh/setup-uv/compare/v3...v4) --- updated-dependencies: - dependency-name: astral-sh/setup-uv dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/build.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 3f66f7f7..1da9d255 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -19,7 +19,7 @@ jobs: # Set fetch-depth: 0 to fetch all history for all branches and tags. fetch-depth: 0 # Needed for setuptools_scm to work correctly - name: Install uv - uses: astral-sh/setup-uv@v3 + uses: astral-sh/setup-uv@v4 - name: Set up Python ${{ matrix.python-version }} uses: actions/setup-python@v5