Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider adding support for nested commands or subcommands #71

Closed
tleonhardt opened this issue Mar 11, 2017 · 14 comments · Fixed by #257
Closed

Consider adding support for nested commands or subcommands #71

tleonhardt opened this issue Mar 11, 2017 · 14 comments · Fixed by #257

Comments

@tleonhardt
Copy link
Member

Many REPL frameworks have support for multi-level commands, where the command has sub-commands.

While this can be done with cmd2, there isn't any built-in support to make it easy.

Consider adding some built-in support to make it easy to have nested commands or subcommands.

@tleonhardt
Copy link
Member Author

tleonhardt commented Jul 6, 2017

@kmvanbrunt
There are a wealth of existing modules which support parsing sub-commands including:

@tleonhardt
Copy link
Member Author

Now that we are using argparse for command argument parsing, doors have been open for this. Users can manually do it using argparse. But maybe we could come up with some sort of clever deocrator-based approach akin to how click approaches subcommands that helps make it easier?

@tleonhardt
Copy link
Member Author

If we can't think of a good way of adding explicit support for nested commands (i.e. subcommands), then perhaps it would be sufficient to simply document by example how to use argparse to do this.

@kotfu kotfu self-assigned this Jan 15, 2018
@kotfu
Copy link
Member

kotfu commented Jan 15, 2018

Since this is possible using argparse, I'll add some documentation and an example or two showing how to do it with argparse. That can be our first step, and then we can reassess whether we should add additional support later.

@lobocv
Copy link
Contributor

lobocv commented Jan 16, 2018

I've come up with a system that allows you to nest Cmd classes within each other. The top level (root) command then handles passing the subcommands down to it's subordinates. You can also "change directory" so that you are at the subcommand's level and therefore do not need to specify the subcommand anymore.

I tried to come up with a decorator like approach but found it to be too complicated. I also liked the ability to separate the subcommands into their own class, it bodes well for code organizing.

I'm not sure how this works with argparse, I don't have much experience with it.
I plan to validate my changes against the test suit today.

Edit: Tests have been completed successfully. I'm now going to focus on adding additional tests for this feature.

@tleonhardt
Copy link
Member Author

@lobocv
Hi Calvin, it is a pleasure to make your acquaintance. Let us know when you are happy with your system that allows you to nest Cmd classes in order to support subcommands and @kotfu and I can take a look at it. We can either take a look at it within a branch on your fork or you can submit a PR. I'm curious to see what you have devised. Having separate Cmd classes for subcommands sounds really non-intuitive to me, but I promise to approach your solution with an open mind, so maybe you can make a convert of me ;-)

argparse has rather nice support for subcommands built into it. I've used it in the past and have been very happy with it. Basically you add a "subparser" to the main parser for each subcommand. It then does a good job of contextually providing help.

@lobocv
Copy link
Contributor

lobocv commented Jan 16, 2018

@tleonhardt Happy to meet you too!

I would have preferred to make a separate SubCommand class (and maybe we can still go that route). My reasoning for having separate classes for each subcommand and then layering them were:

  1. Code cleanliness. When a command line tool gets too large it's easier to read if the subcommands can be grouped together and placed in a separate file.
  2. Modularity. Subcommands can be run as the root / top level command line without any code modifications / relying on a root Cmd class.
  3. Easier to implement (for me at least!) since CLI functions are just redirected to a function of the same name/structure in a subordinate CLI class.

I'm not too familiar with the unit test framework being used in this project so that may take me a little while to figure out. My preliminary tests seem to work and not broken anything!

Please feel free to branch off my fork and play around. I'll add an example here shortly.

I've added a example of nesting commands in examples/subcommands.py (on my fork). Please have a look

@tleonhardt
Copy link
Member Author

@lobocv
pytest is a wonderful unit test framework. If you haven't already checked it out our Contributor's Guide has some good suggestions for using it. In particular, I would highly recommend the xdist plugin for running tests in parallel - it really speeds things up.

Honestly, the vast majority of our tests are integration tests, not unit tests. So please don't take what we have as an example of how to write good unit tests though ;-)

@tleonhardt
Copy link
Member Author

tleonhardt commented Jan 18, 2018

@lobocv
I took a look at what you are doing in your fork on the feature/subcommands branch.

As it turns out what you are doing is totally, radically different from what I have been envisioning when I have talked about a feature that adds support for subcommands.

What I'm talking about is akin to a command such as "git" where that one command has many subcommands - i.e. "git clone", "git status", "git commit", etc. If you "git -h" you will get very different help information than if you do "git clone -h". So effectively it is one command with various subcommands, where each subcommand takes different options, but all are invokable from the same prompt. Except in the context of cmd2 these would be invokable from the cmd2 interactive prompt instead of the shell's command-line.

What you have done is more akin to architecting a way to turn a cmd2-based application into a menu-driven application by having "subcommands" which are really submenus or nested cmd2.Cmd applications.

What you have done is pretty cool and I like it, but I'm not sure I can see a clean way to merge it into the mainstream cmd2 baseline in a way that would be generically useful for and compatible with all other cmd2 applications. Perhaps with some clever work we could. What you have created is a cool feature (though not the same one as this Issue discusses), but you are doing some things which would not be acceptable for general applications, such as altering the prompt. Many cmd2 apps involve code where developers are highly tweaking the prompt, including dynamic updating based on data from background threads and such.

Even if we didn't merge it into the main baseline, maybe we could merge in an example or something. I do think it is a nice example of how to do nested cmd2 apps where you have different commands available in different states.

@kotfu and/or @kmvanbrunt if either of you have a chance to look at his code, I'd like to hear your thoughts.

@lobocv
Copy link
Contributor

lobocv commented Jan 18, 2018

@tleonhardt Thanks for having a look at my work. I had a feeling that my connotation of "subcommand" was slightly different. From now on I will use "submenu" instead of "subcommand". For my job I needed the feature I had made and I thought it would be helpful to contribute it back to the project. I understand the changes are quite significant and can have an adverse affect on people if we merged it. I'm not sure what the best approach would be now.

If the changing of the prompt is a concern, perhaps we can find a solution to that. Currently I alter the prompt to show the "scope" at which the CLI resides. Perhaps, if the prompt is manually changed it, can override this feature.

The library seems to behave the same if you don't use nested Cmd's so perhaps it may not be a problem for people not using this feature.

I'd love to hear everyone's thoughts.

Edit: There may actually be a better way to do this that would not involve altering the cmd2.Cmd class. Instead we can use a decorator that can override the root level Cmd to add do_* commands which perform the redirecting into submenus. This way we shouldn't get any unintended affects by altering the core code and any problems should reside for the most part in the overriding decorator. Perhaps you will be more comfortable with that approach.

@tleonhardt
Copy link
Member Author

@lobocv
I really like the idea of a decorator-based approach for providing the sub-menu feature instead of altering the cmd2.Cmd class itself. I think I would be pretty comfortable merging in such a feature.

@tleonhardt
Copy link
Member Author

@lobocv
I created Issue #256 to explore adding a sub-menu feature. It would probably be a good idea to move future discussion of this feature there since it is logically a different feature than this issue was created to discuss.

@tleonhardt
Copy link
Member Author

PR #257 contains an example of how to use argparse sub-commands as well as a tweak to cmd2 so that it properly supports help for sub-commands.

If anyone has any comments or suggestions for improvement, I'd appreciate hearing them.

@tleonhardt
Copy link
Member Author

@lobocv Just an FYI, you can look at the subcommands.py example that recently got merged into master to see what we were talking about in terms of support for sub-commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants