[MERGE] Make command discovery entirely callback driven.

[Ian Clatworthy]

Robert Collins wrote:
> This makes the act of looking up a command entirely callback driven.

This looks very promising. It needs a bit more polish and perhaps some
more discussion before landing though.

bb:resubmit

> +Internals
> +*********
> +
> +* Command lookup has had hooks added. ``bzrlib.Command.hooks`` has
> +  three new hook points: ``get_command``, ``get_missing_command`` and
> +  ``list_commands``, which allow just-in-time command name provision
> +  rather than requiring all command names be known a-priori.
> +  (Robert Collins)

We're adding 3 new command hooks so I think that's more than an internal
change. I think this ought to be listed under New Features.

> +        self.create_hook(HookPoint('get_command',
> +            "Called when creating a single command. Called with "
> +            "(cmd_or_None, command_name). get_command should either return "
> +            "the cmd_or_None parameter, or a replacement Command object that "
> +            "should be used for the command.", (1, 16), None))
> +        self.create_hook(HookPoint('get_missing_command',
> +            "Called when creating a single command if no command could be "
> +            "found. Called with (command_name). get_missing_command should "
> +            "either return None, or a Command object to be used for the "
> +            "command.", (1, 16), None))
> +        self.create_hook(HookPoint('list_commands',
> +            "Called when enumerating commands. Called with a dict of "
> +            "cmd_name: cmd_class tuples for all the commands found "
> +            "so far. This dict is safe to mutate - to remove a command or "
> +            "to replace it with another (eg plugin supplied) version. "
> +            "list_commands should return the updated dict of commands.",
> +            (1, 16), None))

I'll start here as this is the core of the change. It makes sense to me
now but it didn't at first so I think we need some more doc on several
points here:

1. At first, get_missing_command seemed redundant given get_command.
   I now appreciate that the purpose of get_missing_command is to do
   stuff like "suggest a plugin to install". I can't currently see the
   value in letting it return a command - "notify_missing_command"
   without a return would do I think and make it more obvious to
   developers which hook to use. I also think the *_missing_command
   hooks ought to be processed in *reverse* order of registration, so
   that the standard plugin-db lookup is done very last.

2. The doc for list_commands is wrong - it takes and mutates a set of
   names, not a dict.

3. Maybe we need to explicitly say that there's no need to use these
   (new) hooks when simply registering new commands in plugins.

I think we need to be clearer somehow about the relationship between
list_commands and get_command, given the results can arguably be
inconsistent. I honestly can't see an efficient way of fixing that
though, beyond a dict with lazy instantiation of Command values. OTOH,
the complexity-to-benefit ratio of that is probably dubious?

Thinking out loud, maybe we need to explicitly look at the drivers for
needing the list_commands hook? Shell completion is one and one that we
want to be fast. While outside the scope of this patch, I wonder though
whether shell completion needs further smarts anyway that a plain list
of commands, e.g. detection/expansion of aliases but non-expansion of
hidden commands. In other words, maybe it really needs to check the
command objects anyhow and cache interesting command names? Or maybe
list_commands needs some filtering flags passed as parameters, e.g.
include_hidden?

BTW, the 'resubmit' vote is based on needing to reply to the above,
getting agreement on the hook names/interfaces and the resulting doc
changes. The things below are mainly tweaks ...

> +def _list_bzr_commands(names):
> +    """Return a list of all the registered commands.
> +
> +    This searches plugins and the core.
> +    """

I think the top line ought to say ...

  """Mutates and returns a set of registered command names."""

Should it also search for external commands? (See my comments above re
potential inconsistency between list_commands and get_command.)

> +def all_command_names():
> +    """Return a list of all command names."""

It's actually a set, not list, returned.

> +    names = set()
> +    for hook in Command.hooks['list_commands']:
> +        new_names = hook(names)
> +        if new_names is None:
> +            raise AssertionError(
> +                'hook %s returned None' % Command.hooks.get_hook_name(hook))
> +        names = new_names
> +    return names

I originally though the second last line was a bug and that new_names
ought to be unioned into names. Please add a comment above the line
mentioning that the hook mutates names.

> -def _get_cmd_dict(plugins_override=True):
> -    """Return name->class mapping for all commands."""
> + at deprecated_function(deprecated_in((1, 16, 0)))
> +def get_all_cmds():
> +    """Return canonical name and class for most commands.
> +    
> +    NB: This does not return all commands since the introduction of
> +    command hooks, and returning the class is not sufficient to 
> +    get correctly setup commands, which is why it is deprecated.
> +
> +    Use 'all_command_names' + 'get_cmd_object' instead.
> +    """
>      d = _builtin_commands()
>      if plugins_override:
>          d.update(plugin_cmds.iteritems())
> -    return d

get_all_cmds() has lost it's plugins_override parameter in your
refactoring here.

> +    :raises: KeyError if no command is found.

KeyError can/should go before the : here.

> +        if cmd is not None and not plugins_override:
> +            # We've found a non-plugin command, don't permit it to be
> +            # overridden.
> +            if not cmd.plugin_name():
> +                break

This comment placement is confusing. The top if condition should simply
be extended with ... and not cmd.plugin_name():

> +    if cmd is None:
> +        for hook in Command.hooks['get_missing_command']:
> +            cmd = hook(cmd_name)
> +            if cmd is not None:
> +                break
> +    if cmd is None:
> +        # No command found.
> +        raise KeyError

As mentioned above, I think these ought to be checked in reverse order.
Also, I think an else clause against the for statement is cleaner than a
second 'if cmd is None' test.

> +def _try_plugin_provider(cmd_name):
> +    """Probe for a plugin provider having cmd_name."""
> +    try:
> +        plugin_metadata, provider = probe_for_provider(cmd_name)
> +        raise errors.CommandAvailableInPlugin(cmd_name,
> +            plugin_metadata, provider)
> +    except errors.NoPluginAvailable:
> +        pass

I suspect this will work but it doesn't really meet the documented
get_missing_command hook interface. Maybe the doc needs to be clearer
about propagating exceptions?

> +def _get_external_command(cmd_or_None, cmd_name):
> +    """Lookup a command that is a shell script."""
> +    # Only do external command lookups when no command is found so far.
> +    if cmd_or_None is not None:
> +        return cmd_or_None
> +    from bzrlib.externalcommand import ExternalCommand
>      cmd_obj = ExternalCommand.find_command(cmd_name)
>      if cmd_obj:
>          return cmd_obj

I can't see the value of the final if statement here - just returning
cmd_obj is fine I think. (This isn't something you changed though.)

> +    """Install the hooks to supply bzr's own commands."""
> +    Command.hooks.install_named_hook("list_commands", _list_bzr_commands,
> +        "bzr commands")
> +    Command.hooks.install_named_hook("get_command", _get_bzr_command,
> +        "bzr commands")
> +    Command.hooks.install_named_hook("get_command", _get_plugin_command,
> +        "bzr plugin commands")
> +    Command.hooks.install_named_hook("get_command", _get_external_command,
> +        "bzr external command lookup")

I'd like to see minor tweaks to the labels here for the first 2
get_command hooks: s/commands/command lookup/.

> -        from bzrlib import symbol_versioning
>          symbol_versioning.suppress_deprecation_warnings(override=False)

I'm not sure about dropping this import. The changes to the imports
right at the top don't import symbol_versioning now, just things from
that module. Maybe add suppress_deprecation_warnings to that list?

The tests and other changes look fine. Maybe we need one more test or a
minor enhancement to the existing ones to check processing order when
there are multiple *_missing_command hooks registered though.

Ian C.