[MERGE] Make command discovery entirely callback driven.

[Ian Clatworthy]

Robert Collins wrote:
> On Tue, 2009-05-26 at 16:33 +1000, Ian Clatworthy wrote:

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

Ok.

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

Sure. But there's no reason I can see for a plugin to return a command
via get_missing_command() when it can return one via get_command(). It's
not a big deal and I certainly won't block on having both - I just can't
see why you need get_missing_command() in it's current form.


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

Yes.

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

That's fine with me.

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

Ok.

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

Cool.

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

Actually, it is used in the implementation of get_all_cmds() I reviewed.
See above.

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

Sure.

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

Leaving the ordering to plugins to control is fine by me.

Ian C.