Code review comment for lp:~garyvdm/bzr/register_lazy_decorated

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