Skip to content

Conversation

@anselor
Copy link
Contributor

@anselor anselor commented Apr 10, 2018

Adds ability to optionally group commands into categories:
screenshot_2018-04-10_15-04-53

As well as a way to see more verbose help output:
help -v / help --verbose
screenshot_2018-04-10_14-46-05

This addresses #278

@anselor anselor requested a review from tleonhardt as a code owner April 10, 2018 19:08
cmd2.py Outdated
widest = 0
# measure the commands
for command in cmds:
width = wcswidth(command)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to put this in a conditional check since wcswidth() is not available on Windows. You would so something like the following:

if 'gnureadline' in sys.modules or 'readline' in sys.modules:
    width = wcswidth(command) 
else:
    width = len(command)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out we don't allow arbitrary unicode characters in the command names so I can actually just remove this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is an even better solution.

Copy link
Member

@tleonhardt tleonhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to fix the issue where wcswidth isn't available on Windows.

@@ -0,0 +1,138 @@
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example is nice and helps explain to end users how to use it.

A couple unit tests covering the new features added would be extremely beneficial from a maintenance perspective.

@tleonhardt
Copy link
Member

Thanks for the PR @anselor!

Please fix the dependency issue on Windows.

A few unit tests covering the new features would be greatly appreciated if you don't mind taking the time to add them.

@tleonhardt tleonhardt added this to the 0.8.5 milestone Apr 10, 2018
@tleonhardt
Copy link
Member

If you update the Sphinx docs a little to mention the new features, you get bonus points ;-)

@kmvanbrunt
Copy link
Member

I will need to tweak complete_help() since it isn't expecting a flag.

@kmvanbrunt
Copy link
Member

@anselor @tleonhardt
Nevermind, the complete_help() function is fine with this change.

I do think we need to change what displays in verbose for some commands.
For instance, help says "usage: history [-h] [-r | -e | -s | -o FILE | -t TRANSCRIPT] [arg]"
I would assume it would say, "View, run, edit, and save previously entered commands" from the docstring.

@codecov
Copy link

codecov bot commented Apr 11, 2018

Codecov Report

Merging #348 into master will increase coverage by 0.4%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #348     +/-   ##
=========================================
+ Coverage   89.23%   89.64%   +0.4%     
=========================================
  Files           1        1             
  Lines        1932     2008     +76     
=========================================
+ Hits         1724     1800     +76     
  Misses        208      208
Impacted Files Coverage Δ
cmd2.py 89.64% <100%> (+0.4%) ⬆️

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 322a7c2...e4ca6c8. Read the comment docs.

@anselor
Copy link
Contributor Author

anselor commented Apr 11, 2018

@tleonhardt I don't see anything in the current Sphinx docs covering how to document your commands.

…and> function provided.

In verbose help, added check for argparse usage block (starting with 'usage: '), to skip that block and move to the next comment block
Added unit tests for new categorization code
Updated example to demonstrate skipping of argparse usage statement
@tleonhardt
Copy link
Member

tleonhardt commented Apr 11, 2018

Wow, we really don't. The closest we have is this paragraph in the Argument Processing section.

Feel free to add something earlier ;-)

I think the initial thought was we didn't want to duplicate documentation for features which were implemented and documented within cmd. But now our support for help has grown beyond the base cmd capabilities.

edit history py quit shell unalias
"""

BASE_HELP_VERBOSE = """
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the unit tests. They should really help with maintainability.

Copy link
Member

@tleonhardt tleonhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking pretty good. Thanks for the fixes and updates.

Copy link
Member

@tleonhardt tleonhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test_base_argparse_help unit test is now failing on Python 2.7.

Fix that and then this PR should hopefully be good to go minus potential documentation updates.

…egory.

Changed the detection of with_argparse decorated commands to be less hacky/brittle.
  Now it tags the function with help_summary.
Fixed issue with handling commands that provide a custom help_ function. We can now
redirect the output to a string to be formatted with the other commands.
Added some documentation explaining the new help categories.
Updated unit tests.
@anselor
Copy link
Contributor Author

anselor commented Apr 11, 2018

@tleonhardt I'm looking forward to retiring Python 2 support. The gymnastics to keep it working are annoying.

@tleonhardt
Copy link
Member

@anselor Yeah, I now wish that @kotfu and I had agreed on an earlier date for ending Python 2 support than August 31, 2018. The burden of supporting Python 2.7 is getting worse by the month.

Tell you what, if you can convince @kotfu that moving the date earlier (perhaps May 31, 2018 or even April 30, 2018) is desirable then I am happy to buy-in to that decision. I know moving it earlier would make @kmvanbrunt very happy.

else:
from contextlib import redirect_stdout, redirect_stderr

if sys.version_info > (3, 0):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, using the six module can help avoid Python 2 vs 3 compatibility gymnastics such as this.

six is a Python 2 vs 3 compatibility module (6 == 2 * 3 == 3 * 2).

return lexed_arglist


def with_category(category):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that you added a decorator, that should make the code clean and easy to read.

I strongly encourage you to make one simple change to your decorator to help avoid order-of-operations dependencies when multiple decorators are involved.

You should apply the "@functools.wraps(func)" decorator to your internal cat_decorator function - see how we do it in other decorators below such as with_argument_list:

def with_category(category):
    """A decorator to apply a category to a command function"""
    @functools.wraps(func)
    def cat_decorator(func):
        categorize(func, category)
        return func
    return cat_decorator

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's necessary in this situation because this decorator function is truly a decorator function rather than a wrapping function. This adds an attribute the the passed-in function and returns it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You make a good point, your decorator is mutating and returning the original function. I think you are fine as-is.

Copy link
Member

@tleonhardt tleonhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use functools.wraps in your new decorator to avoid decorator order of application mattering when multiple deorators are used on a function.

tleonhardt
tleonhardt previously approved these changes Apr 11, 2018
Copy link
Member

@tleonhardt tleonhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good to go.

Every now and then the AppVeyor Windows CI tool just gets stuck for no good reason. Can you please push some trivial change to force it to run? Maybe updated the CHANGELOG or something?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants