Merge lp:~ian-clatworthy/bzr/cmds-in-userref-585667 into lp:bzr

Looks plausible to me, but I'm not certain what impact if any this would have on lazy-loading of command objects. I suspect it's fine, but its probably best for Martin or Rob to take a look.

Ian, can I ask what prompted this? It was quite deliberately changed
to not have any commands unless you come in through the command
handler logic.

Frontends *should* be deciding themselves what constitutes a built in command.

-Rob

> Ian, can I ask what prompted this? It was quite deliberately changed
> to not have any commands unless you come in through the command
> handler logic.

Bug 585667. Whomever deliberately changed the semantics of this API didn't grep through the codebase to see what would break. :-( Nor did they document it in NEWS (as far as I can tell) so numerous plugins might be broken by this as well.

> Frontends *should* be deciding themselves what constitutes a built in command.

Perhaps. I'd argue that front-ends ought to have the ability to provide their own built-in commands but falling back to bzr's builtin list is reasonable behaviour. FWIW, there's no public API for loading bzr's builtin commands so clients of the API are currently left up the creek.

One alternative fix to the one suggested would be to make _register_builtin_commands() public and to call it from each and every client, e.g. autodoc_rstx.py, autodoc_man.py. Would that be better in your opinion?

Yes, I'd prefer to have the only implicit-registration code path be
'please run a bzr command'. I'd just call the existing method from the
doc generators - they're in tree, there is absolutely no reason they
shouldn't call the _ function as is.

-Rob

On 26 May 2010 17:20, Ian Clatworthy <email address hidden> wrote:
>> Ian, can I ask what prompted this? It was quite deliberately changed
>> to not have any commands unless you come in through the command
>> handler logic.
>
> Bug 585667. Whomever deliberately changed the semantics of this API didn't grep through the codebase to see what would break. :-( Nor did they document it in NEWS (as far as I can tell) so numerous plugins might be broken by this as well.

Hi, that was probably me, sorry. Thanks for catching it. I did grep
for callers but apparently didn't interpret the results well enough.

The point of the change was to let us load less code up front:
generally speaking not to load command classes until they're really
needed, either to run them or to get their help.

We could clean up a bit more the distinction between just getting the
list of names and loading them. I think this change is not
necessarily wrong but perhaps a better one is possible. If
test_import_tariffs passes I don't object to it.

I'll try to look more at this in a bit.
--
Martin <http://launchpad.net/~mbp/>

I meant to vote needsfixing when I suggested a fix.

> Yes, I'd prefer to have the only implicit-registration code path be
> 'please run a bzr command'. I'd just call the existing method from the
> doc generators - they're in tree, there is absolutely no reason they
> shouldn't call the _ function as is.

I think it is reasonable for there to be a command that says "tell me about all the commands there are" - this may be slow if it has to go out and find them.

> I think it is reasonable for there to be a command that says "tell me about all the commands there are" - this may be slow if it has to go out and find them.

Well there are such commands - bzr commands for instance. This is a
little lower level in the plumbing than that :)

-Rob

This was discussed further with Robert and Martin on IRC and we agreed to land it.

sent to pqm by email

This has conflicts in trunk.

resubmitted from lp:~mbp/bzr/lazy-commands

This actually fails, I would guess because of something related to recent changes for checking commands have help:

======================================================================
FAIL: bzrlib.tests.test_commands.TestCommands.test_no_help_init_failure
----------------------------------------------------------------------
_StringException: Text attachment: log
------------

------------
Text attachment: traceback
------------
Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/testtools/runtest.py", line 128, in _run_user
    return fn(*args)
  File "/usr/lib/python2.6/dist-packages/testtools/testcase.py", line 368, in _run_test_method
    testMethod()
  File "/home/mbp/bzr/lazy-commands/bzrlib/tests/test_commands.py", line 97, in test_no_help_init_failure
    self.assertRaises(ValueError, cmd_foo)
AssertionError: ValueError not raised
------------

This looks like a bad merge in the test_commands file, resurrecting a
deleted test.

Setting this to needs review because it was approved but just needs some Patch Pilot love to finish it off.