Merge lp:~jtv/maas/bug-1189742 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Custom-formatted help output for API operations in the CLI. The default formatting was unhelpful and, in particular, didn't explain how to pass parameters.

Help output for handlers (which do not take parameters) remains unchanged, as does the global help output.

As a simple example, compare the old output:

«
$ ./bin/maas test zone read -h
usage: ./bin/maas test zone read [-h] [-d] [-k] name [data [data ...]]

GET request. Return zone.

positional arguments:
  name
  data

optional arguments:
  -h, --help show this help message and exit
  -d, --debug Display more information about API responses.
  -k, --insecure Disable SSL certificate check
»

...to the new output:

«
$ ./bin/maas test zone read -h
usage: ./bin/maas test zone read [--help] [-d] [-k] name [data [data ...]]

GET request. Return zone.

Positional arguments:
 name

Common command-line options:
    --help, -h
 Show this help message and exit.
    -d, --debug
 Display more information about API responses.
    -k, --insecure
 Disable SSL certificate check
»

The pointless "data" argument is gone, and most sentences are properly capitalised, and spacing is generally a bit more generous. Otherwise, these basic examples should be similar.

A more involved example... Old output:

«
$ ./bin/maas test tag update -h
usage: ./bin/maas test tag update [-h] [-d] [-k] name [data [data ...]]

Update a specific Tag.

positional arguments:
  name
  data

optional arguments:
  -h, --help show this help message and exit
  -d, --debug Display more information about API responses.
  -k, --insecure Disable SSL certificate check

:param name: The name of the Tag to be created. This should be a short
    name, and will be used in the URL of the tag.
:param comment: A long form description of what the tag is meant for.
    It is meant as a human readable description of the tag.
:param definition: An XPATH query that will be evaluated against the
    hardware_details stored for all nodes (output of `lshw -xml`).
»

And new output:

«
$ ./bin/maas test tag update -h
usage: ./bin/maas test tag update [--help] [-d] [-k] name [data [data ...]]

Update a specific Tag.

Positional arguments:
 name

This method accepts keyword arguments. Pass each argument as a
key-value pair with an equals sign between the key and the value:
key1=value1 key2=value key3=value3. Keyword arguments must come after
any positional arguments.

:param name: The name of the Tag to be created. This should be a short
    name, and will be used in the URL of the tag.
:param comment: A long form description of what the tag is meant for.
    It is meant as a human readable description of the tag.
:param definition: An XPATH query that will be evaluated against the
    hardware_details stored for all nodes (output of `lshw -xml`).

Common command-line options:
    --help, -h
 Show this help message and exit.
    -d, --debug
 Display more information about API responses.
    -k, --insecure
 Disable SSL certificate check
»

We could still do better on the usage string, but this was work enough for now!

Jeroen

[Gavin Panella (allenap)]

Bootiful!

review: Approve

=== modified file 'src/maascli/api.py'
--- src/maascli/api.py	2013-10-18 09:32:57 +0000
+++ src/maascli/api.py	2014-02-27 14:50:11 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Interact with a remote MAAS server."""
@@ -16,6 +16,7 @@
     "register_api_commands",
     ]
 
+import argparse
 from collections import defaultdict
 from email.message import Message
 import httplib
@@ -249,6 +250,124 @@
             file.write("\n")
 
 
+class ActionHelp(argparse.Action):
+    """Custom "help" function for an action `ArgumentParser`.
+
+    We use the argument parser's "epilog" field for the action's detailed
+    description.
+
+    This class is stateless.
+    """
+
+    keyword_args_help = dedent("""\
+        This method accepts keyword arguments.  Pass each argument as a
+        key-value pair with an equals sign between the key and the value:
+        key1=value1 key2=value key3=value3.  Keyword arguments must come after
+        any positional arguments.
+        """)
+
+    @classmethod
+    def get_positional_args(cls, parser):
+        """Return an API action's positional arguments.
+
+        Most typically, this holds a URL path fragment for the object that's
+        being addressed, e.g. a physical zone's name.
+
+        There will also be a "data" argument, representing the request's
+        embedded data, but that's of no interest to end-users.  The CLI offers
+        a more fine-grained interface to pass parameters, so as a special case,
+        that one item is left out.
+        """
+        # Use private method on the parser.  The list of actions does not
+        # seem to be publicly exposed.
+        positional_actions = parser._get_positional_actions()
+        names = [action.dest for action in positional_actions]
+        if len(names) > 0 and names[-1] == 'data':
+            names = names[:-1]
+        return names
+
+    @classmethod
+    def get_optional_args(cls, parser):
+        """Return an API action's optional arguments."""
+        # Use private method on the parser.  The list of actions does not
+        # seem to be publicly exposed.
+        optional_args = parser._get_optional_actions()
+        return optional_args
+
+    @classmethod
+    def compose_positional_args(cls, parser):
+        """Describe positional arguments for `parser`, as a list of strings."""
+        positional_args = cls.get_positional_args(parser)
+        if len(positional_args) == 0:
+            return []
+        else:
+            return [
+                '',
+                '',
+                "Positional arguments:",
+                ] + ["\t%s" % arg for arg in positional_args]
+
+    @classmethod
+    def compose_epilog(cls, parser):
+        """Describe action in detail, as a list of strings."""
+        epilog = parser.epilog
+        if parser.epilog is None:
+            return []
+        epilog = epilog.rstrip()
+        if epilog == '':
+            return []
+
+        lines = [
+            '',
+            '',
+            ]
+        if ':param ' in epilog:
+            # This API action documents keyword arguments.  Explain to the
+            # user how those work first.
+            lines.append(cls.keyword_args_help)
+        # Finally, include the actual documentation body.
+        lines.append(epilog)
+        return lines
+
+    @classmethod
+    def compose_optional_args(cls, parser):
+        """Describe optional arguments for `parser`, as a list of strings."""
+        optional_args = cls.get_optional_args(parser)
+        if len(optional_args) == 0:
+            return []
+
+        lines = [
+            '',
+            '',
+            "Common command-line options:",
+            ]
+        for arg in optional_args:
+            # Minimal representation of options.  Doesn't show
+            # arguments to the options, defaults, and so on.  But it's
+            # all we need for now.
+            lines.append('    %s' % ', '.join(arg.option_strings))
+            lines.append('\t%s' % arg.help)
+        return lines
+
+    @classmethod
+    def compose(cls, parser):
+        """Put together, and return, help output for `parser`."""
+        lines = [
+            parser.format_usage().rstrip(),
+            '',
+            parser.description,
+            ]
+        lines += cls.compose_positional_args(parser)
+        lines += cls.compose_epilog(parser)
+        lines += cls.compose_optional_args(parser)
+        return '\n'.join(lines)
+
+    def __call__(self, parser, namespace, values, option_string):
+        """Overridable as defined by the `argparse` API."""
+        print(self.compose(parser))
+        sys.exit(0)
+
+
 def register_actions(profile, handler, parser):
     """Register a handler's actions."""
     for action in handler["actions"]:
@@ -263,9 +382,11 @@
         action_class = type(action_name, action_bases, action_ns)
         action_parser = parser.subparsers.add_parser(
             action_name, help=help_title, description=help_title,
-            epilog=help_body)
-        action_parser.set_defaults(
-            execute=action_class(action_parser))
+            epilog=help_body, add_help=False)
+        action_parser.add_argument(
+            '--help', '-h', action=ActionHelp, nargs=0,
+            help="Show this help message and exit.")
+        action_parser.set_defaults(execute=action_class(action_parser))
 
 
 def register_handler(profile, handler, parser):

=== modified file 'src/maascli/tests/test_api.py'
--- src/maascli/tests/test_api.py	2014-02-26 07:16:56 +0000
+++ src/maascli/tests/test_api.py	2014-02-27 14:50:11 +0000
@@ -18,6 +18,8 @@
 import httplib
 import io
 import json
+import sys
+from textwrap import dedent
 
 import httplib2
 from maascli import api
@@ -37,10 +39,12 @@
     sentinel,
     )
 from testtools.matchers import (
+    EndsWith,
     Equals,
     IsInstance,
     MatchesAll,
     MatchesListwise,
+    Not,
     )
 
 
@@ -264,6 +268,240 @@
         self.assertEqual(response['content'], buf.getvalue())
 
 
+class TestActionHelp(MAASTestCase):
+
+    def make_help(self):
+        """Create an `ActionHelp` object."""
+        option_strings = []
+        dest = 'arg'
+        return api.ActionHelp(option_strings, dest)
+
+    def make_namespace(self):
+        """Return a `namespace` argument that `argparse.Action` will accept."""
+        return factory.make_name('namespace')
+
+    def make_values(self):
+        """Return a `values` argument that `argparse.Action` will accept."""
+        return []
+
+    def make_option_string(self):
+        """Return an `options_string` that `argparse.Action` will accept."""
+        return ""
+
+    def test_get_positional_args_returns_empty_list_if_no_args(self):
+        self.assertEqual(
+            [],
+            api.ActionHelp.get_positional_args(ArgumentParser()))
+
+    def test_get_positional_args_lists_arguments(self):
+        option = factory.make_name('opt', sep='_')
+        parser = ArgumentParser()
+        parser.add_argument(option)
+        self.assertEqual([option], api.ActionHelp.get_positional_args(parser))
+
+    def test_get_positional_args_omits_final_data_arg(self):
+        parser = ArgumentParser()
+        option = factory.make_name('opt', sep='_')
+        parser.add_argument(option)
+        parser.add_argument('data')
+        self.assertEqual([option], api.ActionHelp.get_positional_args(parser))
+
+    def test_get_positional_args_includes_other_arg(self):
+        parser = ArgumentParser()
+        parser.add_argument('data')
+        option = factory.make_name('opt', sep='_')
+        parser.add_argument(option)
+        self.assertEqual(
+            ['data', option],
+            api.ActionHelp.get_positional_args(parser))
+
+    def test_get_positional_args_returns_empty_if_data_is_only_arg(self):
+        parser = ArgumentParser()
+        parser.add_argument('data')
+        self.assertEqual([], api.ActionHelp.get_positional_args(parser))
+
+    def test_get_positional_args_ignores_optional_args(self):
+        parser = ArgumentParser()
+        parser.add_argument('--option')
+        self.assertEqual([], api.ActionHelp.get_positional_args(parser))
+
+    def test_get_optional_args_returns_empty_if_no_args(self):
+        self.assertEqual(
+            [],
+            api.ActionHelp.get_optional_args(ArgumentParser(add_help=False)))
+
+    def test_get_optional_args_returns_optional_args(self):
+        option = '--%s' % factory.make_name('opt')
+        parser = ArgumentParser(add_help=False)
+        parser.add_argument(option)
+        args = api.ActionHelp.get_optional_args(parser)
+        self.assertEqual(
+            [[option]],
+            [action.option_strings for action in args])
+
+    def test_compose_positional_args_returns_empty_if_no_args(self):
+        self.assertEqual(
+            [],
+            api.ActionHelp.compose_positional_args(ArgumentParser()))
+
+    def test_compose_positional_args_describes_positional_args(self):
+        arg = factory.make_name('arg', sep='_')
+        parser = ArgumentParser()
+        parser.add_argument(arg)
+        self.assertEqual(
+            dedent("""\
+
+
+                Positional arguments:
+                \t%s
+                """.rstrip()) % arg,
+            '\n'.join(api.ActionHelp.compose_positional_args(parser)))
+
+    def test_compose_positional_args_does_not_end_with_newline(self):
+        arg = factory.make_name('arg', sep='_')
+        parser = ArgumentParser()
+        parser.add_argument(arg)
+        self.assertThat(
+            '\n'.join(api.ActionHelp.compose_positional_args(parser)),
+            Not(EndsWith('\n')))
+
+    def test_compose_epilog_returns_empty_if_no_epilog(self):
+        self.assertEqual([], api.ActionHelp.compose_epilog(ArgumentParser()))
+
+    def test_compose_epilog_returns_empty_if_epilog_is_empty(self):
+        self.assertEqual(
+            [],
+            api.ActionHelp.compose_epilog(ArgumentParser(epilog='')))
+
+    def test_compose_epilog_returns_empty_if_epilog_is_whitespace(self):
+        self.assertEqual(
+            [],
+            api.ActionHelp.compose_epilog(ArgumentParser(epilog='  \n')))
+
+    def test_compose_epilog_returns_epilog(self):
+        epilog = factory.make_name('epi')
+        self.assertEqual(
+            "\n\n%s" % epilog,
+            '\n'.join(
+                api.ActionHelp.compose_epilog(ArgumentParser(epilog=epilog))))
+
+    def test_compose_epilog_preserves_indentation(self):
+        indent = ' ' * 8
+        epilog = indent + factory.make_name('epi')
+        self.assertEqual(
+            "\n\n%s" % epilog,
+            '\n'.join(
+                api.ActionHelp.compose_epilog(ArgumentParser(epilog=epilog))))
+
+    def test_compose_epilog_explains_documented_keyword_args(self):
+        epilog = ":param foo: The amount of foo."
+        self.assertEqual(
+            '\n\n%s\n%s' % (api.ActionHelp.keyword_args_help, epilog),
+            '\n'.join(
+                api.ActionHelp.compose_epilog(ArgumentParser(epilog=epilog))))
+
+    def test_compose_optional_args_returns_empty_if_none_defined(self):
+        self.assertEqual(
+            [],
+            api.ActionHelp.compose_optional_args(
+                ArgumentParser(add_help=False)))
+
+    def test_compose_optional_args_describes_optional_args(self):
+        long_option = '--%s' % factory.make_name('opt', sep='_')
+        short_option = '-o'
+        option_help = factory.make_name('help')
+        parser = ArgumentParser(add_help=False)
+        parser.add_argument(long_option, short_option, help=option_help)
+        expected_text = dedent("""\
+
+
+            Common command-line options:
+                %s
+            \t%s
+            """) % (', '.join([long_option, short_option]), option_help)
+        self.assertEqual(
+            expected_text.rstrip(),
+            '\n'.join(api.ActionHelp.compose_optional_args(parser)))
+
+    def test_compose_shows_at_least_usage_and_description(self):
+        usage = factory.make_name('usage')
+        description = factory.make_name('description')
+        parser = ArgumentParser(
+            usage=usage, description=description, add_help=False)
+        self.assertEqual(
+            dedent("""\
+                usage: %s
+
+                %s
+                """).rstrip() % (usage, description),
+            api.ActionHelp.compose(parser))
+
+    def test_call_exits(self):
+        parser = ArgumentParser(description=factory.getRandomString())
+        action_help = self.make_help()
+        self.patch(sys, 'exit')
+        self.patch(api, 'print')
+        action_help(
+            parser, self.make_namespace(), self.make_values(),
+            self.make_option_string())
+        self.assertThat(sys.exit, MockCalledOnceWith(0))
+
+    def test_call_shows_full_enchilada(self):
+        usage = factory.make_name('usage')
+        description = factory.make_name('description')
+        epilog = dedent("""\
+            More detailed description here.
+            Typically more than one line.
+            :param foo: The amount of foo.
+            """).rstrip()
+        arg = factory.make_name('arg', sep='_')
+        parser = ArgumentParser(
+            usage=usage, description=description, epilog=epilog,
+            add_help=False)
+        parser.add_argument(arg)
+        option = '--%s' % factory.make_name('opt')
+        option_help = factory.make_name('help')
+        parser.add_argument(option, help=option_help)
+        params = {
+            'usage': usage,
+            'description': description,
+            'arg': arg,
+            'keyword_args_help': api.ActionHelp.keyword_args_help.rstrip(),
+            'epilog': epilog,
+            'option': option,
+            'option_help': option_help,
+            }
+        expected_text = dedent("""\
+            usage: %(usage)s
+
+            %(description)s
+
+
+            Positional arguments:
+            \t%(arg)s
+
+
+            %(keyword_args_help)s
+
+            %(epilog)s
+
+
+            Common command-line options:
+                %(option)s
+            \t%(option_help)s
+            """).rstrip() % params
+        action_help = self.make_help()
+        self.patch(sys, 'exit')
+        self.patch(api, 'print')
+
+        # Invoke ActionHelp.__call__, so we can see what it prints.
+        action_help(
+            parser, self.make_namespace(), self.make_values(),
+            self.make_option_string())
+
+        self.assertThat(api.print, MockCalledOnceWith(expected_text))
+
+
 class TestPayloadPreparation(MAASTestCase):
     """Tests for `maascli.api.Action.prepare_payload`."""
 

=== modified file 'src/maasserver/forms_settings.py'
--- src/maasserver/forms_settings.py	2014-02-26 16:56:23 +0000
+++ src/maasserver/forms_settings.py	2014-02-27 14:50:11 +0000
@@ -263,8 +263,9 @@
         form_details = config_details['form_kwargs']
         doc.append("- " + config_name + ": " + form_details['label'] + ". ")
         # Append help text if present.
-        help_text = form_details.get('help_text', "")
-        doc.append(help_text)
+        help_text = form_details.get('help_text')
+        if help_text is not None:
+            doc.append(help_text.strip())
         # Append list of possible choices if present.
         choices = form_details.get('choices')
         if choices is not None: