[MERGE] Make command discovery entirely callback driven.

[Robert Collins]

On Tue, 2009-05-26 at 16:33 +1000, Ian Clatworthy wrote:
> 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.

These are internal to bzrlib though, I don't think folk using the tool
to develop their own projects care about this sort of stuff.

> 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.

Well there are two things; firstly extend_command still applies to
missing commands, which I think is useful as it allows for UI decoration
to still apply. Secondly, if a missing command is found, then we don't
show the 'no such command' error - and thats important.

As for ordering, I don't think that matters. I can't think of a use case
where an explicit order behaves better than random order.

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

Thanks, fixed.

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

It may be very efficient to use these directly from plugins. I'm ok with
putting some text about this, but I don't want to prejudge what
should/shouldn't be done. Are you worried that folk will see these hooks
and use them rather than the current interface?

> 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?

I definitely don't want list commands to be working with objects. One
long term goal, for instance, is to allow us to remove builtins.py.
Possibly we want a filter on list_commands too, to allow
'list_commands("A*")'. I'm not sure on that, and would rather wait.

> 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?

shell completion builds a cache for the shell by getting the full list
of commmands. So it should be unaltered by this.

> 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."""

How about 
    """Find commands from bzr's core and plugins."""

? I don't think repeating the contract is very useful here - its a very
short function.

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

No, external commands should be handled separately. In fact, I have a
separate effort to move external command support into a plugin using
these hooks.


> > +def all_command_names():
> > +    """Return a list of all command names."""
> 
> It's actually a set, not list, returned.

changed

> > +    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.

Well the hook can either mutate, or just replace. The key thing is that
it returns the value to use. Simplifying into names = hook(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.

Yes. This is deliberate - its not used in the codebase, and there is an
API break already for 1.16. 


> > +    :raises: KeyError if no command is found.
> 
> KeyError can/should go before the : here.

Done.

> > +        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():

Thanks... it was an evolutionary process getting here.

> > +    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.

I don't think that can possibly work, as once you raise you exit the
function.

> Also, I think an else clause against the for statement is cleaner than a
> second 'if cmd is None' test.

It would work but I find 
for (condition):
else:
   foo

sufficiently confusing that I simply refuse to use it as a language
misfeature - even if I remember exactly how it works, the chances are
that a large % of people reading the code later won't.

> > +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?

It doesn't meet the spirit, thats true. I actually meant to refactor
this into a command before submitting, but hadn't gotten around to it.
I'm not sure what you'd like put into the docs though:
"""Like the rest of bzrlib, if you raise an exception it will go up the
call stack.""" ? (My point is that catching exceptions is exceptional,
so surely we should document *that* and not the normal behaviour).

> > +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.)

True. I'm inclined to leave this allow until its just removed :).

> > +    """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?

its globally imported now, and yes, I forgot to add suppress.. to it.
Thanks for catching that.

> 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.

I'm not sure an order is needed. If one is needed then we should allow
plugins to control it entirely.

-Rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090601/eb808c22/attachment.pgp