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
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.
Revision history for this message
Mike Pontillo (mpontillo) wrote :

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
src/maascli/parser.py:61:1: E302 expected 2 blank lines, found 1
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
        should either exit or raise an exception.
        """
        self.print_usage(_sys.stderr)
        self.exit(2, _('%s: error: %s\n') % (self.prog, message))

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
usage: /usr/lib/python2.7/dist-packages/maascli/__main__.py admin device read
       [--help] [-d] [-k] system_id [data [data ...]]
/usr/lib/python2.7/dist-packages/maascli/__main__.py admin device read: error: too few arguments

NEW CODE:

$ bin/maas admin device read
usage: bin/maas admin device read [--help] [-d] [-k]
                                  system_id [data [data ...]]

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

review: Needs Fixing
Revision history for this message
Mike Pontillo (mpontillo) wrote :

I almost forgot; you'll want to add a unit test for this. See src/maascli/tests/test_parser.py.

I'd recommend a test to ensure that the message is printed, and a test to ensure print_help() is called.

Do "bzr grep MockCalledOnceWith" and "bzr grep ' Contains'" to find some examples.

After you've written the tests, you can run "bin/test.cli maascli.tests.test_parser" to run them.

Revision history for this message
Mike Pontillo (mpontillo) wrote :

... and a test to ensure everything is printed on stderr. ;-)

Revision history for this message
Lee Trager (ltrager) wrote :

I've made sure everything is printed to stderr and also print the original error message minus the self.prog. Mike also helped me add two tests, one which checks that print_help is called and another that maas returns 2 when an invalid argument is given.

Hopefully I got everything if not please tell me!

Revision history for this message
Mike Pontillo (mpontillo) wrote :

The code looks better now, so I'll mark it "Approved". But please consider fixing the minor issues with the comments below before landing the branch.

review: Approve
Revision history for this message
Mike Pontillo (mpontillo) wrote :

One more comment about the docstrings.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maascli/parser.py'
2--- src/maascli/parser.py 2015-05-07 18:14:38 +0000
3+++ src/maascli/parser.py 2015-07-11 00:57:56 +0000
4@@ -17,6 +17,7 @@
5 ]
6
7 import argparse
8+import sys
9
10 from maascli import api
11 from maascli.cli import register_cli_commands
12@@ -30,6 +31,14 @@
13 a lazily evaluated `subparsers` property.
14 """
15
16+ def _print_error(self, message):
17+ """Print the specified message to stderr.
18+
19+ This method is used to isolate write to stderr, so that those writes
20+ can be intercepted in a unit test.
21+ """
22+ sys.stderr.write(message)
23+
24 def __init__(self, *args, **kwargs):
25 kwargs.setdefault(
26 "formatter_class", argparse.RawDescriptionHelpFormatter)
27@@ -49,6 +58,17 @@
28 self.__subparsers.metavar = "COMMAND"
29 return self.__subparsers
30
31+ def error(self, message):
32+ """Make the default error messages more helpful
33+
34+ Override default ArgumentParser error method to print the help menu
35+ generated by ArgumentParser instead of just printing out a list of
36+ valid arguments.
37+ """
38+ self.print_help(sys.stderr)
39+ self._print_error('\n' + message + '\n')
40+ sys.exit(2)
41+
42
43 def prepare_parser(argv):
44 """Create and populate an arguments parser for the maascli command."""
45
46=== modified file 'src/maascli/tests/test_parser.py'
47--- src/maascli/tests/test_parser.py 2015-05-07 18:14:38 +0000
48+++ src/maascli/tests/test_parser.py 2015-07-11 00:57:56 +0000
49@@ -14,7 +14,14 @@
50 __metaclass__ = type
51 __all__ = []
52
53-from maascli.parser import ArgumentParser
54+import sys
55+
56+from maascli.parser import (
57+ ArgumentParser,
58+ prepare_parser,
59+)
60+from maastesting.factory import factory
61+from maastesting.matchers import MockCalledOnceWith
62 from maastesting.testcase import MAASTestCase
63
64
65@@ -39,3 +46,35 @@
66 # The subparsers property, once populated, always returns the same
67 # object.
68 self.assertIs(subparsers, parser.subparsers)
69+
70+ def test_bad_arguments_prints_help_to_stderr(self):
71+ argv = ['maas', factory.make_name(prefix="profile"), 'nodes']
72+ parser = prepare_parser(argv)
73+ mock_print_help = self.patch(ArgumentParser, 'print_help')
74+ self.patch(sys.exit)
75+ self.patch(ArgumentParser, '_print_error')
76+ # We need to catch this TypeError, because after our overridden error()
77+ # method is called, argparse expects the system to exit. Without
78+ # catching it, when we mock sys.exit() it will continue unexpectedly
79+ # and crash with the TypeError later.
80+ try:
81+ parser.parse_args(argv[1:])
82+ except TypeError:
83+ pass
84+ self.assertThat(mock_print_help, MockCalledOnceWith(sys.stderr))
85+
86+ def test_bad_arguments_calls_sys_exit_2(self):
87+ argv = ['maas', factory.make_name(prefix="profile"), 'nodes']
88+ parser = prepare_parser(argv)
89+ self.patch(ArgumentParser, 'print_help')
90+ mock_exit = self.patch(sys.exit)
91+ self.patch(ArgumentParser, '_print_error')
92+ # We need to catch this TypeError, because after our overridden error()
93+ # method is called, argparse expects the system to exit. Without
94+ # catching it, when we mock sys.exit() it will continue unexpectedly
95+ # and crash with the TypeError later.
96+ try:
97+ parser.parse_args(argv[1:])
98+ except TypeError:
99+ pass
100+ self.assertThat(mock_exit, MockCalledOnceWith(2))