Merge lp:~ltrager/maas/pretty-incorrect into lp:~maas-committers/maas/trunk
Proposed by
Lee Trager
Status: | Merged |
---|---|
Approved by: | Lee Trager |
Approved revision: | no longer in the source branch. |
Merged at revision: | 4083 |
Proposed branch: | lp:~ltrager/maas/pretty-incorrect |
Merge into: | lp:~maas-committers/maas/trunk |
Diff against target: |
100 lines (+60/-1) 2 files modified
src/maascli/parser.py (+20/-0) src/maascli/tests/test_parser.py (+40/-1) |
To merge this branch: | bzr merge lp:~ltrager/maas/pretty-incorrect |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Mike Pontillo (community) | Approve | ||
Review via email: mp+264368@code.launchpad.net |
Commit message
Show help menu when an invalid argument is given
By default argparse just prints out a list of valid arguments with no reference to what they are. This patch overrides the default argparse function with one that prints out the help menu generated by argparse and exits with the same return code
To post a comment you must log in.
Thanks for proposing this fix, Lee.
While the code looks generally good, there are some subtle issues.
First, our unit testing will also verify:
(1) Correctly formatted imports
(2) PEP 8 compliance
Item (1) can be validated by running "make format" (it will automatically format your imports to our standards, with a few exceptions; basically they should all be in a single section of the file (under str = None), alphabetized. (your code didn't have an issue with this)
For item (2), you need to use "make lint", which will run flake8 and report anything it finds that doesn't align with PEP 8. Here's what it looks like on your branch:
$ make lint parser. py:61:1: E302 expected 2 blank lines, found 1
src/maascli/
make: *** [lint-py] Error 123
If you try to land this as-is, the lander will reject it (due to the unit tests not passing) and set it to "Needs Fixing".
Secondly, you may want to consider overriding error() to print the help text *and* the error message. In the parent class, the function does this:
def error(self, message):
"""error( message: string)
Prints a usage message incorporating the message to stderr and
exits.
If you override this in a subclass, it should not return -- it
self.print_ usage(_ sys.stderr)
self.exit( 2, _('%s: error: %s\n') % (self.prog, message))
should either exit or raise an exception.
"""
The "message" passed into error(), while ugly, may contain important information regarding the user's mistake. So I think you should consider having the overriding method print the help text, then print 'message', *then* exit. Here's a before/after example:
OLD CODE:
$ maas admin device read python2. 7/dist- packages/ maascli/ __main_ _.py admin device read python2. 7/dist- packages/ maascli/ __main_ _.py admin device read: error: too few arguments
usage: /usr/lib/
[--help] [-d] [-k] system_id [data [data ...]]
/usr/lib/
NEW CODE:
$ bin/maas admin device read
system_ id [data [data ...]]
usage: bin/maas admin device read [--help] [-d] [-k]
Read a specific device.
positional arguments:
system_id
data
optional arguments:
--help, -h Show this help message and exit.
-d, --debug Display more information about API responses.
-k, --insecure Disable SSL certificate check
Returns 404 if the device is not found.
So yes, with the new code it looks much nicer, but it could be missing some crucial information.
Note that I think self.prog is ugly, and can be omitted. ;-)
Finally, to be consistent, I think you should print the help (and any error messages) to stderr, so that scripts that error out with incorrect usage don't get data on stdout and incorrectly assume it's output from a command that succeeded.
In summary:
- make format && make lint
- print error message
- print everything to stderr