On Mon, 07 May 2012 20:20:25 -0000, Zygmunt Krynicki <email address hidden> wrote:
> Zygmunt Krynicki has proposed merging lp:~zkrynicki/lava-core/commands-pkg into lp:lava-core with lp:~zkrynicki/lava-core/utils-interface as a prerequisite.
> 
> Requested reviews:
>   Linaro Validation Team (linaro-validation)
> 
> For more details, see:
> https://code.launchpad.net/~zkrynicki/lava-core/commands-pkg/+merge/104974
> 
> This branch adds the essence of lp:lava-tool - fundamental constructs
> for hierarchical command line interface. Again, the code is copied
> from the lava-tool branch that is also merged (via bzr join) to the
> top-level directory of lava-core. It will be removed once we don't
> need standalone lava-tool project for anything (particularly
> lava-test).
> 
> There are some subtle changes from lava-tool's version of Command that
> are worth noting, there is a new method, get_description() that
> supplements get_help() and get_epilog(). This makes the output of
> --help easier to control as the developer can easily inject text in
> three places - right before arguments / option section, right after
> everything else and in any place that lists the description of a
> particular command in one line (such as --help of the command group it
> is a part of).
> 
> There are small bug fixes but they are not worth backporting to
> lp:lava-tool, before it goes away, as they only improve usability
> (again, mainly output of --help) and don't cause any issues today.
> 
> I have kept the "main.py" away from this proposal as it has most of
> the new code and should be reviewed explicitly. Sadly bzr has no `bzr
> cp` so there is no useful diff. I suggest that you just diff from
> lava-tool/ directory.

It seems there has been way too much code motion for this suggestion to
be helpful, unforunately.  lava/core/commands/__init__.py seems to
contain an edited combination of lava-tool/lava/tool/command.py,
lava-tool/lava/tool/dispatcher.py and maybe some other stuff?  I've
basically given up on trying to review this as a diff from existing
code.

> -- 
> https://code.launchpad.net/~zkrynicki/lava-core/commands-pkg/+merge/104974
> You are subscribed to branch lp:lava-core.
> === added directory 'lava/core/commands'
> === added file 'lava/core/commands/__init__.py'
> --- lava/core/commands/__init__.py	1970-01-01 00:00:00 +0000
> +++ lava/core/commands/__init__.py	2012-05-07 20:19:21 +0000
> @@ -0,0 +1,484 @@
> +# Copyright (C) 2011-2012 Linaro Limited
> +#
> +# Author: Zygmunt Krynicki <email address hidden>
> +#
> +# This file is part of lava-core
> +#
> +# lava-core is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU Lesser General Public License version 3
> +# as published by the Free Software Foundation
> +#
> +# lava-core is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public License
> +# along with lava-core.  If not, see <http://www.gnu.org/licenses/>.
> +
> +from __future__ import absolute_import, print_function
> +
> +"""
> +lava.core.commands
> +==================
> +"""
> +
> +import argparse
> +import inspect
> +import re
> +import sys
> +
> +from lava.core.errors import CommandError
> +from lava.core.logging import LoggingMixIn
> +from lava.core.plugins import PluginLoader
> +from lava.core.workspace import Workspace
> +
> +
> +class WorkspaceMixIn(object):

It's from the future!!

> +    """
> +    Mix in for commands that want to interact with the workspace
> +    """
> +
> +    _workspace = None
> +
> +    @property
> +    def workspace(self):
> +        if self._workspace is None:
> +            try:
> +                self._workspace = Workspace.find()
> +            except LookupError:
> +                raise CommandError(("You need to have a workspace to do that, "
> +                                    "see 'lava init --help'"))
> +            try:
> +                self._workspace.load_environment()
> +            except Exception as exc:
> +                raise CommandError(("Unable to load workspace environment: "
> +                                    "{0}").format(exc))
> +        return self._workspace
> +
> +
> +class DisplayMixIn(object):
> +    """
> +    Mix in for commands that want to display something
> +    """
> +
> +    def _list_objects(self, obj_list, format_str, empty=None):
> +        if not obj_list:
> +            if empty is None:
> +                self.say("There are no items in this collection")
> +            else:
> +                self.say(empty)
> +        else:
> +            for obj in obj_list:
> +                self.say(format_str.format(obj=obj))
> +
> +
> +class ExperimentalNoticeAction(argparse.Action):
> +    """
> +    Argparse action that implements the --experimental-notice
> +    """
> +
> +    message = """
> +    Some lc-tool sub-commands are marked as EXPERIMENTAL. Those commands are
> +    not guaranteed to work identically, or have identical interface between
> +    subsequent lc-tool releases.
> +
> +    We do that to make it possible to provide good user interface and
> +    server-side API when working on new features. Once a feature is stabilized
> +    the UI will be frozen and all subsequent changes will retain backwards
> +    compatibility.
> +    """
> +    message = message.lstrip()
> +    message = re.sub(re.compile("[ \t]+", re.M), " ", message)
> +    message = re.sub(re.compile("^ ", re.M), "", message)
> +
> +    def __init__(self,
> +                 option_strings, dest, default=None, required=False,
> +                 help=None):
> +        super(ExperimentalNoticeAction, self).__init__(
> +            option_strings=option_strings, dest=dest, default=default, nargs=0,
> +            help=help)
> +
> +    def __call__(self, parser, namespace, values, option_string=None):
> +        parser.exit(message=self.message)
> +
> +
> +class ExperimentalCommandMixIn(object):
> +    """
> +    Experimental command.
> +
> +    Prints a warning message on each call to invoke()
> +    """
> +
> +    def invoke(self):
> +        self.print_experimental_notice()
> +        return super(ExperimentalCommandMixIn, self).invoke()
> +
> +    @classmethod
> +    def register_arguments(cls, parser):
> +        retval = super(ExperimentalCommandMixIn,
> +                       cls).register_arguments(parser)
> +        parser.register("action", "experimental_notice",
> +                        ExperimentalNoticeAction)
> +        group = parser.add_argument_group("experimental commands")
> +        group.add_argument("--experimental-notice",
> +                            action="experimental_notice",
> +                            default=argparse.SUPPRESS,
> +                            help="Explain the nature of experimental commands")
> +        return retval
> +
> +    def print_experimental_notice(self):
> +        print ("EXPERIMENTAL - SUBJECT TO CHANGE"
> +               " (See --experimental-notice for more info)")
> +
> +
> +class Command(LoggingMixIn):
> +    """
> +    Base class for all command line tool sub-commands.
> +    """
> +
> +    def __init__(self, parser, args):
> +        """
> +        Prepare instance for executing commands.
> +
> +        This method is called immediately after all arguments are parsed and
> +        results are available. This gives subclasses a chance to configure
> +        themselves. The provided parser is an instance of
> +        argparse.ArgumentParser but it may not be the top-level parser (it will
> +        be a parser specific for this command)
> +
> +        The default implementation stores both arguments as instance
> +        attributes.
> +        """
> +        self.parser = parser
> +        self.args = args
> +
> +    def say(self, message, *args, **kwargs):
> +        """
> +        Handy wrapper for print + format
> +        """
> +        self.args.dispatcher.say(self, message, *args, **kwargs)
> +
> +    def invoke(self):
> +        """
> +        Invoke command action.
> +        """
> +        raise NotImplementedError()
> +
> +    def reparse_arguments(self, parser, raw_args):
> +        """
> +        Re-parse raw arguments into normal arguments
> +
> +        Parser is the same as in register_arguments (a sub-parser) The true,

Missing a . after the )?

> +        topmost parser is in self.parser.
> +
> +        This method is only needed for specific commands that need to peek at
> +        the arguments before being able to truly redefine the parser and
> +        re-parse the raw arguments again.
> +        """

I don't think I understand what this is for from the docstring.  Can you
give a silly/short example in it?

> +        raise NotImplementedError()
> +
> +    @classmethod
> +    def get_name(cls):
> +        """
> +        Return the name of this command.
> +
> +        The default implementation strips any leading underscores and replaces
> +        all other underscores with dashes.
> +        """
> +        return cls.__name__.lstrip("_").replace("_", "-")
> +
> +    @classmethod
> +    def get_help(cls):
> +        """
> +        Return the help message of this command
> +
> +        Help is displayed when listing commands available in a
> +        specific group. Help should be short and must fit on one
> +        line. It should be concise yet meaningful as that is what new
> +        users will see to discover available commands.
> +
> +        By default various parts of 'help' are embedded in the
> +        docstring of the class and separated by ^L (the line feed
> +        character). The format is help^Ldescription^Lepilog.
> +
> +        The epilog is optional, description defaults to help if
> +        missing.
> +        """
> +        return cls._parse_docstring()[0]
> +
> +    @classmethod
> +    def get_description(cls):
> +        """
> +        Return the description of this command
> +
> +        Description is displayed in the output of --help, right
> +        between the usage line and the description of arguments.
> +
> +        By default various parts of 'help' are embedded in the
> +        docstring of the class and separated by ^L (the line feed
> +        character). The format is help^Ldescription^Lepilog.
> +
> +        The epilog is optional, description defaults to help if
> +        missing.
> +        """
> +        return cls._parse_docstring()[1]
> +
> +    @classmethod
> +    def get_epilog(cls):
> +        """
> +        Return the epilog of the help message
> +
> +        Epilog is always displayed last, after all of the other
> +        things that --help prints. It should contain a long
> +        discussion of the command and the way it is intended to be
> +        used. It is a perfect place to put a few short examples.
> +
> +        By default various parts of 'help' are embedded in the
> +        docstring of the class and separated by ^L (the line feed
> +        character). The format is help^Ldescription^Lepilog.
> +
> +        The epilog is optional, description defaults to help if
> +        missing.
> +        """
> +        return cls._parse_docstring()[2]
> +
> +    @classmethod
> +    def _parse_docstring(cls):
> +        doc = inspect.getdoc(cls)
> +        help = desc = doc
> +        epilog = None
> +        if doc is not None:
> +            if doc.count("") == 1:
> +                help, epilog = doc.split("", 1)
> +                desc = help
> +            elif doc.count("") > 1:
> +                help, desc, epilog = doc.split("", 2)
> +        return help, desc, epilog
> +
> +    @classmethod
> +    def get_formatter_class(cls):
> +        """
> +        Return the argparse formatter class that should be used
> +        with this command.
> +        """
> +        return argparse.RawDescriptionHelpFormatter
> +
> +    @classmethod
> +    def register_arguments(cls, parser):
> +        """
> +        Register arguments if required.
> +
> +        Subclasses can override this to add any arguments that will be
> +        exposed to the command line interface.
> +        """
> +        pass
> +
> +    def alter_process_name(self, set_fn, get_fn):
> +        """
> +        Change process name if required.
> +
> +        This method may not becalled on all platforms. Where available set_fn

becalled -> be called

This seems kinda specialized somehow.

> +        and get_fn allow to set and get (read) the current process name.
> +        """
> +        pass
> +
> +
> +class CommandGroup(Command):
> +    """
> +    Base class for all command groups.
> +
> +    This class is needed when one wants to get a custom level of command
> +    options that can be freely extended, just like the top-level `lava`
> +    command.
> +
> +    For example, a CommandGroup 'actions' will load additional commands from a
> +    the 'lava.actions' namespace. For the end user it will be available as::
> +
> +        $ lava foo actions xxx
> +
> +    Where xxx is one of the Commands that is declared to live in the namespace
> +    provided by 'foo actions'.

This docstring seems kinda muddled.  I know your English is way better
than this usually :-) Where does the foo come from in the example?

> +    """
> +
> +    namespace = None
> +
> +    @classmethod
> +    def get_namespace(cls):
> +        """
> +        Return the pkg-resources entry point namespace name from which
> +        sub-commands will be loaded.
> +        """
> +        return cls.namespace
> +
> +    @classmethod
> +    def register_subcommands(cls, parser):
> +        """
> +        Register sub commands.
> +
> +        This method is called around the same time as register_arguments()
> +        would be called for the plain command classes. It loads commands from
> +        the entry point namespace returned by get_namespace() and registeres
> +        them with a Dispatcher class. The parsers used by that dispatcher
> +        are linked to the calling dispatcher parser so the new commands enrich
> +        the top-level parser tree.
> +
> +        In addition, the provided parser stores a dispatcher instance in its
> +        defaults. This is useful when one wants to access it later. To a final
> +        command instance it shall be available as self.args.dispatcher.
> +        """
> +        dispatcher = Dispatcher(parser, name=cls.get_name(),
> +                                help=cls.get_help(),
> +                                description=cls.get_description(),
> +                                epilog=cls.get_epilog())
> +        namespace = cls.get_namespace()
> +        if namespace is not None:
> +            dispatcher.import_commands(namespace)
> +        parser.set_defaults(dispatcher=dispatcher)
> +
> +
> +class Dispatcher(LoggingMixIn):
> +    """
> +    Class implementing command line interface for launch control

Heh, I think this docstring could perhaps be updated :)

> +    """
> +
> +    help = None
> +    description = None
> +    epilog = None
> +
> +    def __init__(self, parser=None, name=None, help=None,
> +                 description=None, epilog=None):
> +        self.name = name
> +        self.help = help or self.__class__.help
> +        self.description = description or self.__class__.description
> +        self.epilog = epilog or self.__class__.epilog
> +        self.parser = parser or self.construct_parser()
> +        self.subparsers = self.parser.add_subparsers(
> +                title="sub-command to invoke")
> +
> +    def __repr__(self):
> +        return "%r(name=%r)" % (self.__class__.__name__, self.name)
> +
> +    def construct_parser(self):
> +        """
> +        Construct a parser for this dispatcher.
> +
> +        This is only used if the parser is not provided by the parent
> +        dispatcher instance.
> +        """
> +        parser_args = dict(add_help=True)
> +        # Set description based on class description
> +        if self.description is not None:
> +            parser_args['description'] = self.description
> +        # Set help based on the class help
> +        if self.help is not None:
> +            parser_args['help'] = self.help
> +        # Set the epilog based on class epilog
> +        if self.epilog is not None:
> +            parser_args['epilog'] = self.epilog
> +        # Customize text formatter so that long/multiline description
> +        # looks better.
> +        parser_args['formatter_class'] = argparse.RawDescriptionHelpFormatter
> +        # Return the fresh parser
> +        return argparse.ArgumentParser(**parser_args)
> +
> +    def import_commands(self, entrypoint_name):
> +        """
> +        Import commands from given entry point namespace
> +        """
> +        loader = PluginLoader(entrypoint_name, Command)
> +        for command_name, command_cls in loader.load_all_plugins().iteritems():
> +            self.add_command_cls(command_cls)
> +
> +    def add_command_cls(self, command_cls):
> +        """
> +        Add a new command class to this dispatcher.
> +
> +        The command must be a subclass of Command or SubCommand.
> +        """
> +        self.logger.debug("Loading command class %r", command_cls)
> +        # Create a sub-parser where the command/sub-command can register
> +        # things.
> +        sub_parser = self.subparsers.add_parser(
> +            command_cls.get_name(),
> +            help=command_cls.get_help(),
> +            formatter_class=command_cls.get_formatter_class(),
> +            description=command_cls.get_description(),
> +            epilog=command_cls.get_epilog()
> +        )
> +        if issubclass(command_cls, CommandGroup):
> +            # Handle CommandGroup somewhat different. Instead of calling
> +            # register_arguments we call register_subcommands
> +            command_cls.register_subcommands(sub_parser)
> +            # Let's also call register arguments in case we need both
> +            command_cls.register_arguments(sub_parser)
> +        else:
> +            # Handle plain commands by recording their commands in the
> +            # dedicated sub-parser we've crated for them.
> +            command_cls.register_arguments(sub_parser)
> +            # In addition, since we don't want to require all sub-classes of
> +            # Command to super-call register_arguments (everyone would forget
> +            # this anyway) we manually register the command class for that
> +            # sub-parser so that dispatch() can look it up later.
> +            sub_parser.set_defaults(
> +                command_cls=command_cls,
> +                parser=sub_parser)
> +        # Make sure the sub-parser knows about this dispatcher
> +        sub_parser.set_defaults(dispatcher=self)
> +
> +    def _adjust_logging_level(self, args):
> +        """
> +        Adjust logging level after seeing the initial arguments
> +        """

What is this for?  Is it just for subclasses to override?

> +    def dispatch(self, raw_args=None, reraise_command_errors=False):
> +        """
> +        Dispatch a command with the specified arguments.
> +
> +        If arguments are left out they are looked up in sys.argv automatically
> +        """
> +        # First parse whatever input arguments we've got
> +        args = self.parser.parse_args(raw_args)
> +        # Adjust logging level after seeing arguments
> +        self._adjust_logging_level(args)
> +        # Then look up the command class and construct it with the parser it
> +        # belongs to and the parsed arguments.
> +        command = args.command_cls(args.parser, args)
> +        try:
> +            # Give the command a chance to re-parse command line arguments
> +            command.reparse_arguments(args.parser, raw_args)
> +        except NotImplementedError:
> +            pass
> +        # Give the command a chance to change process name if we have the
> +        # required support module.
> +        try:
> +            from setproctitle import getproctitle, setproctitle
> +        except ImportError:
> +            self.history.warning(
> +                ("Unable to set import 'setproctitle', "
> +                 "process name cannot be changed"))
> +        else:
> +            command.alter_process_name(
> +                get_fn=getproctitle, set_fn=setproctitle)
> +        with self.history.debug("Dispatching command: {command}",
> +                                command=raw_args or sys.argv):
> +            try:
> +                return command.invoke()
> +            except CommandError as exc:
> +                print("ERROR: %s" % (exc,), file=sys.stderr)
> +                if reraise_command_errors:
> +                    raise
> +                return 1
> +
> +    @classmethod
> +    def run(cls, args=None):
> +        """
> +        Instantiate a dispatcher, dispatch a command and exit
> +        """
> +        raise SystemExit(cls().dispatch(args))
> +
> +    def say(self, command, message, *args, **kwargs):
> +        """
> +        Handy wrapper for print + format
> +        """
> +        print(message.format(*args, **kwargs))
> 
> === modified file 'lava/core/errors.py'
> --- lava/core/errors.py	2012-05-07 20:19:21 +0000
> +++ lava/core/errors.py	2012-05-07 20:19:21 +0000
> @@ -36,3 +36,11 @@
>      """
>      Error raised when a plugin cannot be loaded
>      """
> +
> +
> +class CommandError(Exception):
> +    """
> +    Raise this from a Command's invoke() method to display an error nicely.
> +
> +    `lava` will exit with a status of 1 if this is raised.
> +    """
> 

Mostly seems fine!  I'm not that familiar with argparse so I'll just
have to take your word for it that it all works.

Cheers,
mwh