[MERGE] Make command discovery entirely callback driven.
ian.clatworthy at internode.on.net
Mon Jun 15 06:58:02 BST 2009
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.
>> 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?
> 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."""
>> 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.
>>> + 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:
>>> - 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.
>>> +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 :).
>> 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.
More information about the bazaar