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

[Ian Clatworthy]

Ensure builtin_command_names() is initialized correctly

bzrlib.commands.builtin_command_names() is "broken" in the trunk in that it now requires special initialisation. Rather than fixing each client, this patch ensures the initialisation is always done.

[Andrew Bennetts (spiv)]

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.

review: Abstain

[Robert Collins (lifeless)]

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 Clatworthy (ian-clatworthy)]

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

[Robert Collins (lifeless)]

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

[Martin Pool (mbp)]

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

[Robert Collins (lifeless)]

I meant to vote needsfixing when I suggested a fix.

review: Needs Fixing

[Martin Pool (mbp)]

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

[Robert Collins (lifeless)]

> 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

[Ian Clatworthy (ian-clatworthy)]

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

[Robert Collins (lifeless)]

sent to pqm by email

[Robert Collins (lifeless)]

This has conflicts in trunk.

[Martin Pool (mbp)]

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

[Martin Pool (mbp)]

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

[Robert Collins (lifeless)]

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

[John A Meinel (jameinel)]

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

=== modified file 'bzrlib/commands.py'
--- bzrlib/commands.py	2010-05-11 11:05:49 +0000
+++ bzrlib/commands.py	2010-05-26 02:26:22 +0000
@@ -222,6 +222,7 @@
     Use of all_command_names() is encouraged rather than builtin_command_names
     and/or plugin_command_names.
     """
+    _register_builtin_commands()
     return builtin_command_registry.keys()
 
 

=== modified file 'bzrlib/tests/test_commands.py'
--- bzrlib/tests/test_commands.py	2010-05-11 11:05:49 +0000
+++ bzrlib/tests/test_commands.py	2010-05-26 02:26:22 +0000
@@ -84,6 +84,11 @@
             pass
         self.assertRaises(ValueError, cmd_foo)
 
+    def test_builtin_command_names(self):
+        cmds = commands.builtin_command_names()
+        self.assertTrue(len(cmds) > 0)
+        self.assertTrue('diff' in cmds)
+
 
 class TestGetAlias(tests.TestCase):
 
@@ -339,6 +344,7 @@
         self.assertEqual(['called'], hook_calls)
         self.assertSubset(['foo', 'bar'], cmds)
 
+
 class TestDeprecations(tests.TestCase):
 
     def test_shlex_split_unicode_deprecation(self):