yellow team mailing list archive

[Francesco Banconi <francesco.banconi@xxxxxxxxxxxxx>]

Francesco Banconi has proposed merging lp:~frankban/lpsetup/help-fixes into lp:lpsetup.

Requested reviews:
  Yellow Squad (yellow)

For more details, see:
https://code.launchpad.net/~frankban/lpsetup/help-fixes/+merge/120957

= Summary =

This branch introduces 2 main changes:

1) Remove step names repetition in subcommands help.

This is achieved easily by overriding the `metavar` while creating the steps actions. By default, argparse generates the metavar (the value displayed as example in the help for a specific argument) using choices where present. Now the metavar is just 'STEP_NAME', and the choices are explicitly listed in the action's help.

2) Make subcommands help more descriptive, adding usage examples.

This is less trivial. 

I added a custom argparse HelpFormatter that preserves newlines and tabs. The default one just replaces \s+ with a single space and the uses textwrap.fill() to format the resulting text. Now textwrap.fill() is used on each line, and tabs are preserved: they are used to indent command line examples.

I also introduced the concept of "epilog" in the subcommands protocol, as an optional name. If a subcommand defines an epilog, that value is used when creating the subparser. The epilogs are already supported by the Python argparse library, and represent texts to be added at the end of the auto-generated help messages.

The global help (i.e. `lp-setup help`) epilog is generated collecting subcommands' epilogs. `cli.SUBCOMMANDS` is used to sort all the epilogs. As a consequence, I reordered the subcommands list so that the generated help message makes sense.

Added an epilog for each subcommand: this strongly increased this MP diff.


== Other changes ==

The help now is wrapped at terminal width. Looking at the argparse code I've seen that HelpFormatter can be instantiated passing a width, representing the number of columns used to wrap the help message. We already have a reliable function that calculates the terminal width, so, I thought this could be a nice improvement.

In the help of optional argument attached to the parser by subcommands we often had the default value explicitly added, e.g. help='the help [DEFAULT={0}]'.format(default_value). I've found that argparse automatically formats the help strings passing a context that already contains the default value. So this branch replaces the line above with: help='the help [DEFAULT=%(default)s]'.
-- 
https://code.launchpad.net/~frankban/lpsetup/help-fixes/+merge/120957
Your team Yellow Squad is requested to review the proposed merge of lp:~frankban/lpsetup/help-fixes into lp:lpsetup.

=== modified file 'lpsetup/argparser.py'
--- lpsetup/argparser.py	2012-08-13 10:30:06 +0000
+++ lpsetup/argparser.py	2012-08-23 10:13:32 +0000
@@ -30,6 +30,10 @@
         - add_arguments(parser): a callable that receives a parser and can
           add subcommand specific arguments to that parser.
 
+        - epilog: a string representing text to be added at the end of the
+          auto-generated help message. The text can contain `%(prog)s` as a
+          placeholder to current subcommand name, e.g 'lpsetup init-target'.
+
         - has_dry_run: True if the subcommand supports dry run execution,
           False otherwise. If not defined, False is considered.
 
@@ -156,6 +160,29 @@
 from lpsetup.utils import get_terminal_width
 
 
+class HelpFormatter(argparse.HelpFormatter):
+    """A customized help formatter for `argparse`.
+
+    This formatter wraps text preserving new lines and tabs.
+    """
+
+    def _fill_text(self, text, width, indent):
+        """Return text wrapped preserving new lines and tabs."""
+        lines = []
+        for line in text.splitlines():
+            # The default formatter here replaces newlines, multiple spaces
+            # and tabs with a single space. Newlines are not present here
+            # (since we already called *splitlines()*) and we want to preserve
+            # tabs, so we only need to remove multiple whitespaces.
+            cleaned = ' '.join(filter(None, line.split(' ')))
+            # Wrap the line at given *width*.
+            wrapped = textwrap.fill(
+                cleaned, width,
+                initial_indent=indent, subsequent_indent=indent)
+            lines.append(wrapped)
+        return '\n'.join(lines)
+
+
 class ArgumentParser(argparse.ArgumentParser):
     """A customized argument parser for `argparse`."""
 
@@ -175,6 +202,13 @@
         action = super(ArgumentParser, self).add_argument(*args, **kwargs)
         self.actions.append(action)
 
+    def _get_formatter(self):
+        """Override to use a customized help formatter.
+
+        Also create the formatter instance wrapping help at terminal width.
+        """
+        return HelpFormatter(prog=self.prog, width=get_terminal_width())
+
 
 class BaseSubCommand(object):
     """Base class defining a subcommand.
@@ -297,11 +331,18 @@
         """Add steps related arguments."""
         step_names = [get_step_name(i[0]) for i in self.steps]
         parser.add_argument(
-            '-s', '--steps', nargs='+', choices=step_names,
-            help='Call one or more internal functions.')
+            '-s', '--steps',
+            choices=step_names,  metavar='STEP_NAME', nargs='+',
+            # argparse internally formats the help string using choices
+            # if they are present as an attribute of the action.
+            help='Call one or more internal functions. '
+                 'Available steps for this subcommand, in the order they '
+                 'are executed: %(choices)s.')
         parser.add_argument(
-            '--skip-steps', nargs='+', choices=step_names,
-            help='Skip one or more internal functions.')
+            '--skip-steps',
+            choices=step_names, metavar='STEP_NAME', nargs='+',
+            help='Skip one or more internal functions. '
+                 'See -s, --steps help for a list of available steps.')
 
     def get_description(self, namespace):
         """Collect steps' descriptions and return them as a single string."""

=== modified file 'lpsetup/cli.py'
--- lpsetup/cli.py	2012-08-15 20:50:45 +0000
+++ lpsetup/cli.py	2012-08-23 10:13:32 +0000
@@ -13,6 +13,7 @@
     ]
 
 import sys
+
 from lpsetup import (
     __doc__ as description,
     argparser,
@@ -30,13 +31,15 @@
 from lpsetup.utils import confirm
 
 
+# The global help epilog, built collecting subcommands' epilogs,
+# reflects the order of the following sequence.
 SUBCOMMANDS = [
-    ('finish-init-target', finish_init_target.SubCommand()),
+    ('init-lxc', initlxc.SubCommand()),
     ('init-target', init_target.SubCommand()),
-    ('init-lxc', initlxc.SubCommand()),
     ('init-repo', initrepo.SubCommand()),
+    ('update', update.SubCommand()),
+    ('finish-init-target', finish_init_target.SubCommand()),
     ('install-lxc', install_lxc.SubCommand()),
-    ('update', update.SubCommand()),
     ('version', version.SubCommand()),
     ]
 
@@ -124,19 +127,37 @@
 def get_parser(subcommands):
     """Return the argument parser with all subcommands registered.
 
-    Also add the help subcommand.
+    Also add the help subcommand and the global epilog generated collecting
+    all subcommands' epilogs.
     """
     parser = argparser.ArgumentParser(description=description)
     subparsers = parser.add_subparsers(
         title='subcommands',
         help='Each subcommand accepts --h or --help to describe it.')
+    epilogs = ['%(prog)s provides several subcommands that can be used to '
+               'create and update Launchpad development and testing '
+               'environments. Examples of how to use the available '
+               'subcommands follow.']
     for name, subcommand in subcommands:
+        # Prepare the subcommand help and epilog.
+        help = subcommand.help
+        epilog = getattr(subcommand, 'epilog', None)
+        if epilog is not None:
+            # Retrieve the program name including base name and subcommand.
+            prog = '{0} {1}'.format(parser.prog, name)
+            epilogs.extend([
+                '=== {0} ==='.format(prog),
+                epilog % {'prog': prog}
+                ])
         # Register the subcommand.
-        help = subcommand.help
-        subparser = subparsers.add_parser(name, description=help, help=help)
+        subparser = subparsers.add_parser(
+            name, description=help, help=help, epilog=epilog)
         # Prepare the subcommand arguments.
         prepare_parser(subparser, subcommand)
 
+    # Add the global epilog.
+    parser.epilog = '\n\n'.join(epilogs)
+
     # Add the *help* subcommand.
     def main(namespace):
         command = namespace.command

=== modified file 'lpsetup/subcommands/finish_init_target.py'
--- lpsetup/subcommands/finish_init_target.py	2012-08-15 23:34:34 +0000
+++ lpsetup/subcommands/finish_init_target.py	2012-08-23 10:13:32 +0000
@@ -68,6 +68,24 @@
         )
     handlers = (handle_user, handle_target_dir)
 
+    epilog = """`%(prog)s` finalizes the Launchpad environment set up.
+        This subcommand must be run once the Launchpad code is retrieved.
+
+        \tcd mybranch
+        \t$ %(prog)s
+
+        This is completely equivalent to the following command:
+
+        \t$ %(prog)s mybranch
+
+        A user name is required to set up the Launchpad database, and by \
+        default the current system user name is used. If instead this \
+        subcommand is run as root, the user name needs to be explicitly \
+        specified:
+
+        \t$ %(prog)s -u myuser
+    """
+
     def add_arguments(self, parser):
         super(SubCommand, self).add_arguments(parser)
         parser.add_argument(

=== modified file 'lpsetup/subcommands/init_target.py'
--- lpsetup/subcommands/init_target.py	2012-08-15 23:34:34 +0000
+++ lpsetup/subcommands/init_target.py	2012-08-23 10:13:32 +0000
@@ -305,6 +305,33 @@
         handle_ssh_keys,
         )
 
+    epilog = """`%(prog)s` prepares a Launchpad environment in the current \
+        machine, setting up ssh keys if needed and initializing Launchpad \
+        dependencies. The machine can either be your local system, an LXC \
+        container or a cloud instance. However, please refer to \
+        `lp-setup init-lxc` for an more convenient way to set up a Launchpad \
+        environment inside an LXC.
+
+        Prepare a machine to run Launchpad:
+
+        \t$ %(prog)s
+
+        By default the home of the current user is configured, but if this \
+        subcommand is run as root, you need to explicitly specify the user:
+
+        \t# %(prog)s -u myuser
+
+        In this case, if `myuser` does not exist, it will be created.
+
+        The current user (or the provided one) should have a correctly \
+        configured bazaar user id (i.e `bzr whoami`). However, sometimes this \
+        is not possible (e.g. when the provided user does not exist). In this \
+        case, you can explicitly pass the user's full name and email and \
+        the bazaar user id will be set up using provided arguments:
+
+        \t# %(prog)s -u myuser -f 'My User' -e myuser@xxxxxxxxxxx
+    """
+
     def add_arguments(self, parser):
         super(SubCommand, self).add_arguments(parser)
         parser.add_argument(
@@ -328,6 +355,5 @@
                  'user name is used.')
         parser.add_argument(
             '-S', '--ssh-key-name', default=SSH_KEY_NAME,
-            help='{0} [DEFAULT={1}]'.format(
-                'The ssh key name used to connect to Launchpad.',
-                SSH_KEY_NAME))
+            help='The ssh key name used to connect to Launchpad. '
+                 '[DEFAULT=%(default)s]')

=== modified file 'lpsetup/subcommands/initlxc.py'
--- lpsetup/subcommands/initlxc.py	2012-08-16 09:59:20 +0000
+++ lpsetup/subcommands/initlxc.py	2012-08-23 10:13:32 +0000
@@ -264,22 +264,58 @@
     help = __doc__
     root_required = True
 
+    epilog = """`%(prog)s` creates an LXC container bind mounting the home \
+        directory of the current user, setting up ssh keys if needed and \
+        initializing Launchpad dependencies.
+
+        Create and initialize an LXC named `mylxc`:
+
+        \t$ %(prog)s mylxc
+
+        The default LXC name is `{lxc_name}`, so, to create and initialize \
+        an LXC named `{lxc_name}`, you can avoid passing the name:
+
+        \t$ %(prog)s
+
+        By default the home of the current user is bind mounted, but if this \
+        subcommand is run as root, you need to explicitly specify the user:
+
+        \t# %(prog)s -u myuser
+
+        In this case, if `myuser` does not exist, it will be created.
+
+        The current user (or the provided one) should have a correctly \
+        configured bazaar user id (i.e `bzr whoami`). However, sometimes this \
+        is not possible (e.g. when the provided user does not exist). In this \
+        case, you can explicitly pass the user's full name and email and \
+        the bazaar user id will be set up using provided arguments:
+
+        \t# %(prog)s -u myuser -f 'My User' -e myuser@xxxxxxxxxxx
+
+        At the end of the process the newly created LXC instance is running, \
+        and it is possible to obtain its ip address using:
+
+        \t$ sudo lp-lxc-ip -n mylxc
+
+        If instead you want `%(prog)s` to stop the instance once it is \
+        completely configured, you can use the `--stop-lxc` argument:
+
+        \t$ %(prog)s --stop-lxc
+    """.format(lxc_name=LXC_NAME)
+
     def add_arguments(self, parser):
         super(SubCommand, self).add_arguments(parser)
         # Add LXC related arguments.
         parser.add_argument(
             'lxc_name', nargs='?', default=LXC_NAME,
-            help='The LXC container name to setup. '
-                 '[DEFAULT={0}]'.format(LXC_NAME))
+            help='The LXC container name to setup. [DEFAULT=%(default)s]')
         parser.add_argument(
             '-A', '--lxc-arch', default=LXC_GUEST_ARCH,
-            help='The LXC container architecture. '
-                 '[DEFAULT={0}]'.format(LXC_GUEST_ARCH))
+            help='The LXC container architecture. [DEFAULT=%(default)s]')
         parser.add_argument(
             '-R', '--lxc-os', default=LXC_GUEST_OS,
             choices=LXC_GUEST_CHOICES,
-            help='The LXC container distro codename. '
-                 '[DEFAULT={0}]'.format(LXC_GUEST_OS))
+            help='The LXC container distro codename. [DEFAULT=%(default)s]')
         parser.add_argument(
             '--stop-lxc', action='store_true',
             help='Shut down the LXC container at the end of the process.')

=== modified file 'lpsetup/subcommands/initrepo.py'
--- lpsetup/subcommands/initrepo.py	2012-08-09 15:32:08 +0000
+++ lpsetup/subcommands/initrepo.py	2012-08-23 10:13:32 +0000
@@ -35,7 +35,9 @@
     LP_BRANCH_NAME,
     LP_BZR_LOCATIONS,
     LP_CHECKOUT_NAME,
+    LP_HTTP_REPO,
     LP_REPOSITORY_DIR,
+    LP_SSH_REPO,
     )
 from lpsetup.utils import (
     call,
@@ -148,6 +150,44 @@
         )
     root_required = False
 
+    epilog = """Once a Launchpad environment is correctly initialized \
+        (i.e. using `init-lxc` or `init-target`) `%(prog)s` can be used \
+        to retrieve the Launchpad code and its dependencies.
+
+        By default, the code is retrieved from `{ssh_repo}`, a shared \
+        repository without trees is created in `{repository}`, containing a \
+        lightweight checkout named `{checkout}`:
+
+        \t$ %(prog)s
+
+        However, it is possible to change these parameters, e.g. to create a \
+        repository in `~/mylaunchpad` containing a lightweight checkout \
+        `mycheckout` and retrieving code from `lp:~myuser/launchpad/mybranch`:
+
+        \t$ %(prog)s --source lp:~myuser/launchpad/mybranch -r ~/mylaunchpad \
+        --checkout-name mycheckout
+
+        As seen above, the default behavior is to get the code using bzr+ssh: \
+        we assume you set up a valid Launchpad user id. This way you can \
+        contribute to Launchpad, commit code, fix bugs, etc. Anyway, \
+        sometimes an http checkout can be more convenient (e.g. when creating \
+        testing environments). To retrieve Launchpad from `{http_repo}`:
+
+        \t$ %(prog)s --use-http
+
+        Lightweight checkouts are considered an effective way of working with \
+        bazaar repositories. On the other hand, developers may prefer other \
+        approaches (normal branches with trees, co-located branches). It is \
+        possible to initialize a Launchpad repository with trees as follow:
+
+        \t$ %(prog)s --no-checkout --branch-name trunk
+
+        If `--branch-name` is not provided `{branch}` is used by default.
+    """.format(
+        branch=LP_BRANCH_NAME, checkout=LP_CHECKOUT_NAME,
+        http_repo=LP_HTTP_REPO, repository=LP_REPOSITORY_DIR,
+        ssh_repo=LP_SSH_REPO)
+
     @staticmethod
     def add_common_arguments(parser):
         parser.add_argument(
@@ -161,23 +201,22 @@
             '--source is provided.  Defaults to off.')
         parser.add_argument(
             '--branch-name', default=LP_BRANCH_NAME,
-            help='Name of the directory in which to place the branch.  '
-                'Defaults to {0}.'.format(LP_BRANCH_NAME))
+            help='Name of the directory in which to place the branch. '
+                 '[DEFAULT=%(default)s]')
         parser.add_argument(
             '--checkout-name', default=LP_CHECKOUT_NAME,
-            help='Create a checkout with the given name. '
-                 'Ignored if --no-checkout is specified. '
-                 'Defaults to {0}.'.format(LP_CHECKOUT_NAME))
+            help='Create a checkout with the given name. Ignored if '
+                 '--no-checkout is specified. [DEFAULT=%(default)s]')
         parser.add_argument(
             '-r', '--repository', default=LP_REPOSITORY_DIR,
             help='The directory of the Launchpad repository to be created. '
                  'The directory must reside under the home directory of the '
-                 'given user (see -u argument). '
-                 '[DEFAULT={0}]'.format(LP_REPOSITORY_DIR))
+                 'given user (see -u argument). [DEFAULT=%(default)s]')
         parser.add_argument(
             '--no-checkout', action='store_true', dest='no_checkout',
-            default=False, help='Initialize a bzr repository with trees and '
-            'do not create a lightweight checkout.')
+            default=False,
+            help='Initialize a bzr repository with trees and '
+                 'do not create a lightweight checkout.')
 
     def add_arguments(self, parser):
         super(SubCommand, self).add_arguments(parser)

=== modified file 'lpsetup/subcommands/install_lxc.py'
--- lpsetup/subcommands/install_lxc.py	2012-08-16 09:59:20 +0000
+++ lpsetup/subcommands/install_lxc.py	2012-08-23 10:13:32 +0000
@@ -158,6 +158,28 @@
         )
     help = __doc__
 
+    epilog = """`%(prog)s` sets up a complete Launchpad environment in an LXC \
+    container, creating the LXC, configuring the instance, retrieving the code.
+
+    This is basically the same of running `init-lxc` and then, inside the \
+    newly created container, `init-repo`, `update` and `finish-init-target`.
+
+    Set up Launchpad for the current user:
+
+    \t%(prog)s
+
+    A more complex example: set up an environment for `myuser`, creating a \
+    repository in a customized path, using a different ssh key and activating \
+    the parallel testing tweaks.
+
+    \t%(prog)s -u myuser -E myuser@xxxxxxxxxxx -f 'My User' \
+    -r ~/my-lp-branches -S launchpad_lxc_id_rsa --testing
+
+    %(prog)s inherits its arguments from the subcommands it wraps. \
+    You can refer to the help of all the other subcommands for a more \
+    exaustive explanation.
+    """
+
     def get_handlers(self, namespace):
         handlers = super(SubCommand, self).get_handlers(namespace)
         return handlers + (

=== modified file 'lpsetup/subcommands/update.py'
--- lpsetup/subcommands/update.py	2012-08-09 09:17:34 +0000
+++ lpsetup/subcommands/update.py	2012-08-23 10:13:32 +0000
@@ -98,6 +98,25 @@
         handlers.handle_target_dir,
         )
 
+    epilog = """`%(prog)s` updates an existing Launchpad branch, contextually \
+        retrieving the current external sources and dependencies.
+
+        To update a branch in the current working directory:
+
+        \tcd ~/launchpad/lp-branches/mybranch
+        \t%(prog)s
+
+        To update a branch specifying its path:
+
+        \t%(prog)s ~/launchpad/lp-branches/mybranch
+
+        By default this subcommand updates the download cache checking out \
+        {deps}. To change the source repository for the cache, use \
+        `--lp-source-deps`:
+
+        \t%(prog)s --lp-source-deps my-source-deps
+    """.format(deps=LP_SOURCE_DEPS)
+
     @staticmethod
     def add_common_arguments(parser):
         parser.add_argument(
@@ -107,8 +126,7 @@
                  'working-dir. ')
         parser.add_argument(
             '--lp-source-deps', default=LP_SOURCE_DEPS,
-            help='URL of LP source deps branch.  '
-                 '[DEFAULT={0}]'.format(LP_SOURCE_DEPS))
+            help='URL of LP source deps branch. [DEFAULT=%(default)s]')
 
     def add_arguments(self, parser):
         super(SubCommand, self).add_arguments(parser)

=== modified file 'lpsetup/tests/test_argparser.py'
--- lpsetup/tests/test_argparser.py	2012-08-13 09:44:02 +0000
+++ lpsetup/tests/test_argparser.py	2012-08-23 10:13:32 +0000
@@ -8,6 +8,7 @@
 from collections import OrderedDict
 import itertools
 import sys
+import textwrap
 import unittest
 
 from lpsetup import argparser
@@ -222,6 +223,34 @@
         self.assertStartsWith('  ', second_line)
 
 
+class HelpFormatterTest(unittest.TestCase):
+
+    def test_newlines(self):
+        # Ensure the customized help formatter wraps text preserving new lines.
+        formatter = argparser.HelpFormatter('prog', width=25)
+        wrapped = 'This text will be wrapped.'
+        preserved = '\nNew lines\nare preserved.'
+        formatter.add_text(wrapped + preserved)
+        expected = textwrap.fill(wrapped, 25) + preserved + '\n'
+        self.assertEqual(expected, formatter.format_help())
+
+    def test_tabs(self):
+        # Ensure the customized help formatter wraps text preserving tabs.
+        formatter = argparser.HelpFormatter('prog', width=80)
+        text = '\tTabs are\tpreserved.'
+        formatter.add_text(text)
+        self.assertEqual(text.expandtabs() + '\n', formatter.format_help())
+
+    def test_whitespaces(self):
+        # Ensure the customized help formatter replaces multiple whitespaces
+        # with a single whitespace, and drop leading and trailing whitespaces.
+        formatter = argparser.HelpFormatter('prog', width=80)
+        text = ' Text with leading, trailing and  multiple   whitespaces.  '
+        formatter.add_text(text)
+        expected = ' '.join(text.split()) + '\n'
+        self.assertEqual(expected, formatter.format_help())
+
+
 class InitNamespaceTest(unittest.TestCase):
 
     def test_user_info_in_namespace(self):

=== modified file 'lpsetup/tests/test_cli.py'
--- lpsetup/tests/test_cli.py	2012-08-13 08:42:08 +0000
+++ lpsetup/tests/test_cli.py	2012-08-23 10:13:32 +0000
@@ -64,6 +64,26 @@
         # The help subcommand also prints arguments info.
         self.assertIn('--foo', output_value)
 
+    def test_epilog_in_global_help(self):
+        # The epilog attribute of subcommands is used to generate
+        # the base command usage message.
+        subcommand = self.make_subcommand(epilog='epilog message')
+        parser = cli.get_parser([('foo', subcommand)])
+        help = parser.format_help()
+        self.assertIn(subcommand.epilog, help)
+
+    def test_subcommand_epilog(self):
+        # The epilog attribute of subcommands is used to create the parser,
+        # so that the epilog text is included in the subcommand help.
+        name = 'foo'
+        subcommand = self.make_subcommand(epilog='epilog message')
+        parser = cli.get_parser([(name, subcommand)])
+        namespace = parser.parse_args(['help', name])
+        with self.assertRaises(SystemExit):
+            with capture_output() as output:
+                namespace.main(namespace)
+        self.assertIn(subcommand.epilog, output.getvalue())
+
     def test_subcommands(self):
         # Ensure sub parsers are added for each registered subcommand.
         subcommands = (