Merge lp:~garyvdm/bzr/register_lazy_decorated into lp:~bzr/bzr/trunk-old

Make it possible to register decorate commands lazily.

Fixes Bug 304154.

thanks for working on this.

some minor notes:
I'd prefer if this doesn't use exception handling to decide
what the previous command is, rather check it:

- try:
+ if key in self:
             previous = self.get(key)
- except KeyError:
+ else:
             previous = _builtin_commands().get(key

There are some lines longer than 79 chars:
commands.py line 116 and 138.
There are also a bunch of blank lines with whitespace, which can be cleaned up.

other than that it looks fine to me.
(I haven't actually tried out this functionality yet, but I'd like to soon.)
bb:tweak

On Sun, 2009-07-12 at 21:08 +0000, Marius Kruger wrote:
> thanks for working on this.
>
> some minor notes:
> I'd prefer if this doesn't use exception handling to decide
> what the previous command is, rather check it:
>
> - try:
> + if key in self:
> previous = self.get(key)
> - except KeyError:
> + else:
> previous = _builtin_commands().get(key

This requires two lookups ('key in self' and 'self.get') vs one. Whats
the objects to an exception?

(Another way to write it that has neither an exception nor double
lookups is;

previous = self.get(key, None)
if previous is None:
    previous = _builtin_commands().get(key)

A further issue is that _builtin_cmds + self.get isn't a full set of
commands. It would better to call
try:
    previous = get_cmd_object(cmd_name)
except errors.BzrCommandError:
    previous = None

except that this will trigger the missing command lookup hook. I'll look
at fixing that hook to support this use case (though I'm rather of the
opinion that its overly protective these days).

For now though, to be robust, you should say 'if name in
all_command_names()'.

-Rob

Is it possible to put your test helpers 'decorated_command' and
'fake_command' into the test_commands python module? There is no need to
have them separate as far as I can tell, and if you are relying on them
not being imported when the test runs - well python won't permit you to
rely on that for a number of reasons.

'fake' as a command name is one that might actually get used someday.
How about using one of the already used test command names rather than
increasing the overlap between testing namespace and real?

I commented in another mail about the use of _builtin_cmds(); I see that
you're actually copying another use of it. How about turning the
existing use into a helper call? This will prevent code duplicatin.

I think I'd rather see *both* uses of it go away, but thats a different
proposition and I'm writing mail to the list about it now.

-Rob

2009/7/12 Robert Collins <email address hidden>:
> On Sun, 2009-07-12 at 21:08 +0000, Marius Kruger wrote:
...
>> I'd prefer if this doesn't use exception handling to decide
>> what the previous command is, rather check it:
>>
>> -        try:
>> +        if key in self:
>>              previous = self.get(key)
>> -        except KeyError:
>> +        else:
>>              previous = _builtin_commands().get(key
>
> This requires two lookups ('key in self' and 'self.get') vs one. Whats
> the objects to an exception?

Maybe its just some of my java baggage talking, but in general I don't think
its good practice to use exception handling to determine normal code flow.
It can have hidden side effects, like catching unrelated KeyErrors
or have additional exception-creating-overhead, although I do agree
that it is sometimes more efficient just to try something in stead of
checking if you can.
Anyway if you're ok with this, its fine.

In python its considered better to ask forgiveness rather than
permission. The pattern of checking first, 'look before you leap' is
often associated with poor performance and/or complex code.

-Rob

Based on previous discussion