Load/Unload commands at runtime - Implements #943 #945

anselor · 2020-06-15T16:52:34Z

Group commands in CommandSets to be installed/uninstalled as a batch. Commands within a CommandSet can be optionally tagged with at default category. Individual commands may override the default category.
Install/Uninstall individual command functions at runtime
Tag command functions for automatic discovery and load
Automatic discovery of complete_ and help_ functions that are contained in the same module as command functions.
Automatic discovery and load of CommandSets and registered command functions that can be disabled in the constructor

codecov · 2020-06-15T17:02:37Z

Codecov Report

Merging #945 into master will decrease coverage by 0.05%.
The diff coverage is 96.13%.

@@            Coverage Diff             @@
##           master     #945      +/-   ##
==========================================
- Coverage   97.71%   97.66%   -0.06%     
==========================================
  Files          17       18       +1     
  Lines        4155     4318     +163     
==========================================
+ Hits         4060     4217     +157     
- Misses         95      101       +6

Impacted Files	Coverage Δ
cmd2/cmd2.py	`96.73% <94.96%> (-0.17%)`	⬇️
cmd2/__init__.py	`92.00% <100.00%> (+0.33%)`	⬆️
cmd2/command_definition.py	`100.00% <100.00%> (ø)`
cmd2/utils.py	`98.25% <0.00%> (+0.24%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa92771...25d6ed4. Read the comment docs.

tleonhardt · 2020-06-16T00:58:24Z

On my laptop I'm getting the following error trying to build the Sphinx docs using Python 3.8.2 and Sphinx 3.1.1:

  File "/Users/toddleonhardt/src/cmd2/.venv/lib/python3.8/site-packages/sphinx/util/logging.py", line 421, in filter
    raise exc
sphinx.errors.SphinxWarning: /Users/toddleonhardt/src/cmd2/cmd2/cmd2.py:docstring of cmd2.Cmd::py:class reference target not found: cmd2.command_definition.CommandSet

tleonhardt

I took a quick look, but I need to take a more in-depth look later.

This is a significant change.

@kmvanbrunt @kotfu I would appreciate feedback from both of you on this one as well.

cmd2/cmd2.py

cmd2/command_definition.py

anselor · 2020-06-16T02:30:52Z

On my laptop I'm getting the following error trying to build the Sphinx docs using Python 3.8.2 and Sphinx 3.1.1:

  File "/Users/toddleonhardt/src/cmd2/.venv/lib/python3.8/site-packages/sphinx/util/logging.py", line 421, in filter
    raise exc
sphinx.errors.SphinxWarning: /Users/toddleonhardt/src/cmd2/cmd2/cmd2.py:docstring of cmd2.Cmd::py:class reference target not found: cmd2.command_definition.CommandSet

That's weird. I ran sphinx on mine and didn't have any problems. I'm running Ubuntu 20.04 through WSL with the latest. I'll have to check when I get back on that machine what the versions were.

tleonhardt

@anselor I started giving this code an in-depth review and have made a number of comments and suggestions for change. I sincerely apologize for not getting to this PR sooner - life has been busy.

Overall this adds a feature that I've thought for a long time we need a good example to show developers how to do it - namely how to define commands in separate files and/or dynamically load commands at runtime.

However, it adds a lot of complexity and does so in a way I fear may be challenging for others to maintain. A few thoughts that passed through my mind while reviewing include the following:

This could probably be done more simply
There is more than one way to achieve this and I'm not sure we should pick one way as the "right" way
This a great excellent example for users with very nice work put into test, a good working example, and documentation ... but would it more appropriately exist as a cmd2 extension akin to cmd2-ext-test than as a core part of cmd2?

I'm not sure what the right answers are? What are your thoughts?

@kmvanbrunt and @kotfu If either of you are available to review this PR and/or weigh in with your opinion, I would very much appreciate it.

tleonhardt · 2020-07-07T01:32:54Z

cmd2/cmd2.py

+                 auto_load_commands: bool = True) -> None:
        """An easy but powerful framework for writing line-oriented command
        interpreters. Extends Python's cmd package.



It is critical that you document these new parameters in the docstring below because that is what populates the Sphinx documentation.

cmd2/cmd2.py

tleonhardt · 2020-07-07T01:41:56Z

cmd2/cmd2.py

                                                shortcuts=shortcuts)

+        # Load modular commands
+        self._installed_functions = []  # type: List[str]


Is this the right data structure? More importantly does it need to exist at all?

The only place this is used is in install_command_functiion() where it is appended to and in uninstall_command() where it is checked to see if the specified command name is in this list?

If we keep it, it should be a set so that there is fast O(1) test for membership.

However, it looks like it is very weakly used and I'm not entirely sure if it is necessary.

tleonhardt · 2020-07-07T01:45:38Z

cmd2/cmd2.py


+        # Load modular commands
+        self._installed_functions = []  # type: List[str]
+        self._installed_command_sets = []  # type: List[CommandSet]


This too should be a set for the fast membership test since it is only ever used to see if something is in this container.

It is also weakly used and I'm not sure it is strictly necessary.

tleonhardt · 2020-07-07T02:00:29Z

cmd2/cmd2.py

+    def _autoload_commands(self) -> None:
+        """
+        Load modular command definitions.
+        :return: None


Specifying :return: in docstring is redundant when using type hints and should not be done unless you are providing more context on what is returned.

This comment applies throughout this PR

tleonhardt · 2020-07-07T02:50:08Z

examples/modular_commands_main.py

@@ -0,0 +1,127 @@
+#!/usr/bin/env python


You forgot to make this file executable - make sure to chmod it

That's weird. It shows up as executable for me.

tleonhardt · 2020-07-07T02:53:38Z

examples/modular_commands_main.py

+
+
+class WithCommandSets(Cmd):
+    def __init__(self, *args, **kwargs):


Please don't follow this anti-pattern of just using *args, **kwargs. It makes code harder to read and maintain. Please explicitly list out the args you care about, e.g. command_sets. All arguments to the parent class __init__() are optional so it is safe to pass in the one you care about.

tleonhardt · 2020-07-07T02:59:00Z

examples/modular_commands_main.py

+
+from cmd2 import Cmd, Cmd2ArgumentParser, CompletionItem, with_argparser
+from cmd2.utils import CompletionError, basic_complete
+from modular_commands.commandset_basic import BasicCompletionCommandSet  # noqa: F401


I think documentation at the top of the example of how BasicCompletionCommandSet gets loaded is in order because it probably isn't intuitive to someone just look at this example.

tleonhardt · 2020-07-07T03:00:07Z

examples/modular_commands/commandset_basic.py

@@ -0,0 +1,121 @@
+# coding=utf-8


Creating an empty init.py file in this directory might be a good enough hack to get the examples to run but probably not to work nicely in an IDE

Yeah, the layout of the examples doesn't really support this very well.

tleonhardt · 2020-07-07T03:00:54Z

cmd2/__init__.py

@@ -23,6 +23,7 @@
 # Get the current value for argparse_custom.DEFAULT_ARGUMENT_PARSER


Before merging in this would need a CHANGELOG entry for version 1.2.0 or whatever.

Issue #943 New class CommandSet can be used to tag a class as a command class. If the constructor is simple, the object will automatically be instantiated and loaded. New register_command decorator to tag any arbitrary function as a command.

…tation for CommandSet commands. Issue #943

…oad. Added unit tests. Moved installing commands into separate functions that can be called Issue #943

Issue #943

…ding command functions Adds handling of some edge cases. More thorough test coverage.

…e functions.

…d PyCharm

…e are scenarios where inspect.ismethod() fails for some reason

anselor · 2020-08-04T17:39:39Z

Accepting #961 instead.

anselor requested review from kmvanbrunt and tleonhardt June 15, 2020 16:52

anselor requested a review from kotfu as a code owner June 15, 2020 16:52

tleonhardt reviewed Jun 16, 2020

View reviewed changes

anselor force-pushed the commandset branch from aac638c to b007331 Compare June 16, 2020 18:50

tleonhardt requested changes Jul 7, 2020

View reviewed changes

anselor force-pushed the commandset branch from 2a75050 to 30d90a7 Compare July 7, 2020 17:21

anselor force-pushed the commandset branch from af93f7f to d661421 Compare July 17, 2020 22:50

anselor and others added 13 commits July 18, 2020 16:41

Some minor cleanup of how imports work. Fixed issue with help documen…

744cfbf

…tation for CommandSet commands. Issue #943

Added new constructor parameter to flag whether commands should autol…

8300fa1

…oad. Added unit tests. Moved installing commands into separate functions that can be called Issue #943

add ability to remove commands and commandsets

a92e261

Issue #943

Fixes issue with locating help_ annd complete_ functions when autoloa…

8a55193

…ding command functions Adds handling of some edge cases. More thorough test coverage.

Fixes to sphinx generation

1490e9d

Added explicit tests for dir and setattr. Minor type hinting changes

564ffdf

Added more command validation. Moved some common behavior into privat…

35c37b8

…e functions.

Appears to be a type hinting olution that works for flake, sphinx, an…

07b2ec1

…d PyCharm

Sort imports using isort

be70feb

cleanup

ba7f85d

Adjusted decorators to accept variable positional parameters

1cd01c2

Added an additional check for isinstance(method, Callable) since ther…

91a9ed5

…e are scenarios where inspect.ismethod() fails for some reason

anselor force-pushed the commandset branch from b63906b to 91a9ed5 Compare July 18, 2020 20:41

added additional documentation for new decorator behavior

c794059

anselor closed this Aug 4, 2020

anselor deleted the commandset branch August 4, 2020 17:39



		class WithCommandSets(Cmd):
		def __init__(self, args, *kwargs):

		@@ -23,6 +23,7 @@
		# Get the current value for argparse_custom.DEFAULT_ARGUMENT_PARSER

Load/Unload commands at runtime - Implements #943 #945

Load/Unload commands at runtime - Implements #943 #945

Uh oh!

Conversation

anselor commented Jun 15, 2020

Uh oh!

codecov bot commented Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tleonhardt commented Jun 16, 2020

Uh oh!

tleonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anselor commented Jun 16, 2020

Uh oh!

tleonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anselor commented Aug 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jun 15, 2020 •

edited

Loading