Merge lp:~lifeless/bzr/Commands.hooks into lp:~bzr/bzr/trunk-old

Proposed by Robert Collins on 2009-06-01
Status: Merged
Merged at revision: not available
Proposed branch: lp:~lifeless/bzr/Commands.hooks
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 510 lines (has conflicts)
Text conflict in NEWS
To merge this branch: bzr merge lp:~lifeless/bzr/Commands.hooks
Reviewer Review Type Date Requested Status
Ian Clatworthy 2009-06-01 Needs Fixing on 2009-06-01
Review via email: mp+6936@code.launchpad.net
To post a comment you must log in.
Robert Collins (lifeless) wrote :

This makes command lookup fully hookified.

Why? It makes the core logic much clearer IMO, and also makes it
possible to skin bzrlib more completely for things like loggerhead and
olive want to have their own command (not subcommand) but still want to
use bzr's Command and option handling infrastructure.

-Rob

Ian Clatworthy (ian-clatworthy) wrote :

This was reviewed (via BB) last week and my review comments haven't been actioned yet. See https://lists.ubuntu.com/archives/bazaar/2009q2/058746.html.

review: Needs Fixing

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-06-12 08:39:44 +0000
3+++ NEWS 2009-06-15 08:36:49 +0000
4@@ -184,9 +184,19 @@
5 Internals
6 *********
7
8+<<<<<<< TREE
9 * Remove ``weave.py`` script for accessing internals of old weave-format
10 repositories. (Martin Pool)
11
12+=======
13+* Command lookup has had hooks added. ``bzrlib.Command.hooks`` has
14+ three new hook points: ``get_command``, ``get_missing_command`` and
15+ ``list_commands``, which allow just-in-time command name provision
16+ rather than requiring all command names be known a-priori.
17+ (Robert Collins)
18+
19+
20+>>>>>>> MERGE-SOURCE
21 Testing
22 *******
23
24
25=== modified file 'bzrlib/commands.py'
26--- bzrlib/commands.py 2009-06-10 03:56:49 +0000
27+++ bzrlib/commands.py 2009-06-15 08:36:49 +0000
28@@ -49,10 +49,11 @@
29 )
30 """)
31
32-from bzrlib import registry
33-# Compatibility
34 from bzrlib.hooks import HookPoint, Hooks
35+# Compatibility - Option used to be in commands.
36 from bzrlib.option import Option
37+from bzrlib import registry
38+from bzrlib.symbol_versioning import deprecated_function, deprecated_in
39
40
41 class CommandInfo(object):
42@@ -133,98 +134,181 @@
43
44 def _builtin_commands():
45 import bzrlib.builtins
46+ return _scan_module_for_commands(bzrlib.builtins)
47+
48+
49+def _scan_module_for_commands(module):
50 r = {}
51- builtins = bzrlib.builtins.__dict__
52- for name in builtins:
53+ for name, obj in module.__dict__.iteritems():
54 if name.startswith("cmd_"):
55 real_name = _unsquish_command_name(name)
56- r[real_name] = builtins[name]
57+ r[real_name] = obj
58 return r
59
60
61+def _list_bzr_commands(names):
62+ """Return a list of all the registered commands.
63+
64+ This searches plugins and the core.
65+ """
66+ # to eliminate duplicates
67+ names.update(builtin_command_names())
68+ names.update(plugin_command_names())
69+ return names
70+
71+
72+def all_command_names():
73+ """Return a list of all command names."""
74+ names = set()
75+ for hook in Command.hooks['list_commands']:
76+ new_names = hook(names)
77+ if new_names is None:
78+ raise AssertionError(
79+ 'hook %s returned None' % Command.hooks.get_hook_name(hook))
80+ names = new_names
81+ return names
82+
83+
84 def builtin_command_names():
85- """Return list of builtin command names."""
86+ """Return list of builtin command names.
87+
88+ Use of all_command_names() is encouraged rather than builtin_command_names
89+ and/or plugin_command_names.
90+ """
91 return _builtin_commands().keys()
92
93
94 def plugin_command_names():
95+ """Returns command names from commands registered by plugins."""
96 return plugin_cmds.keys()
97
98
99-def _get_cmd_dict(plugins_override=True):
100- """Return name->class mapping for all commands."""
101+@deprecated_function(deprecated_in((1, 16, 0)))
102+def get_all_cmds():
103+ """Return canonical name and class for most commands.
104+
105+ NB: This does not return all commands since the introduction of
106+ command hooks, and returning the class is not sufficient to
107+ get correctly setup commands, which is why it is deprecated.
108+
109+ Use 'all_command_names' + 'get_cmd_object' instead.
110+ """
111 d = _builtin_commands()
112 if plugins_override:
113 d.update(plugin_cmds.iteritems())
114- return d
115-
116-
117-def get_all_cmds(plugins_override=True):
118- """Return canonical name and class for all registered commands."""
119- for k, v in _get_cmd_dict(plugins_override=plugins_override).iteritems():
120+ for k, v in d.iteritems():
121 yield k,v
122
123
124 def get_cmd_object(cmd_name, plugins_override=True):
125- """Return the canonical name and command class for a command.
126+ """Return the command object for a command.
127
128 plugins_override
129 If true, plugin commands can override builtins.
130 """
131 try:
132- cmd = _get_cmd_object(cmd_name, plugins_override)
133- # Allow plugins to extend commands
134- for hook in Command.hooks['extend_command']:
135- hook(cmd)
136- return cmd
137+ return _get_cmd_object(cmd_name, plugins_override)
138 except KeyError:
139 raise errors.BzrCommandError('unknown command "%s"' % cmd_name)
140
141
142 def _get_cmd_object(cmd_name, plugins_override=True):
143- """Worker for get_cmd_object which raises KeyError rather than BzrCommandError."""
144- from bzrlib.externalcommand import ExternalCommand
145+ """Get a command object.
146
147+ :param cmd_name: The name of the command.
148+ :param plugins_override: Allow plugins to override builtins.
149+ :return: A Command object instance
150+ :raises: KeyError if no command is found.
151+ """
152 # We want only 'ascii' command names, but the user may have typed
153 # in a Unicode name. In that case, they should just get a
154 # 'command not found' error later.
155 # In the future, we may actually support Unicode command names.
156-
157- # first look up this command under the specified name
158- if plugins_override:
159+ cmd = None
160+ # Get a command
161+ for hook in Command.hooks['get_command']:
162+ cmd = hook(cmd, cmd_name)
163+ if cmd is not None and not plugins_override:
164+ # We've found a non-plugin command, don't permit it to be
165+ # overridden.
166+ if not cmd.plugin_name():
167+ break
168+ if cmd is None:
169+ for hook in Command.hooks['get_missing_command']:
170+ cmd = hook(cmd_name)
171+ if cmd is not None:
172+ break
173+ if cmd is None:
174+ # No command found.
175+ raise KeyError
176+ # Allow plugins to extend commands
177+ for hook in Command.hooks['extend_command']:
178+ hook(cmd)
179+ return cmd
180+
181+
182+def _try_plugin_provider(cmd_name):
183+ """Probe for a plugin provider having cmd_name."""
184+ try:
185+ plugin_metadata, provider = probe_for_provider(cmd_name)
186+ raise errors.CommandAvailableInPlugin(cmd_name,
187+ plugin_metadata, provider)
188+ except errors.NoPluginAvailable:
189+ pass
190+
191+
192+def probe_for_provider(cmd_name):
193+ """Look for a provider for cmd_name.
194+
195+ :param cmd_name: The command name.
196+ :return: plugin_metadata, provider for getting cmd_name.
197+ :raises NoPluginAvailable: When no provider can supply the plugin.
198+ """
199+ # look for providers that provide this command but aren't installed
200+ for provider in command_providers_registry:
201 try:
202- return plugin_cmds.get(cmd_name)()
203- except KeyError:
204+ return provider.plugin_for_command(cmd_name), provider
205+ except errors.NoPluginAvailable:
206 pass
207- cmds = _get_cmd_dict(plugins_override=False)
208+ raise errors.NoPluginAvailable(cmd_name)
209+
210+
211+def _get_bzr_command(cmd_or_None, cmd_name):
212+ """Get a command from bzr's core."""
213+ cmds = _builtin_commands()
214 try:
215 return cmds[cmd_name]()
216 except KeyError:
217 pass
218- if plugins_override:
219- for key in plugin_cmds.keys():
220- info = plugin_cmds.get_info(key)
221- if cmd_name in info.aliases:
222- return plugin_cmds.get(key)()
223 # look for any command which claims this as an alias
224 for real_cmd_name, cmd_class in cmds.iteritems():
225 if cmd_name in cmd_class.aliases:
226 return cmd_class()
227-
228+ return cmd_or_None
229+
230+
231+def _get_external_command(cmd_or_None, cmd_name):
232+ """Lookup a command that is a shell script."""
233+ # Only do external command lookups when no command is found so far.
234+ if cmd_or_None is not None:
235+ return cmd_or_None
236+ from bzrlib.externalcommand import ExternalCommand
237 cmd_obj = ExternalCommand.find_command(cmd_name)
238 if cmd_obj:
239 return cmd_obj
240
241- # look for plugins that provide this command but aren't installed
242- for provider in command_providers_registry:
243- try:
244- plugin_metadata = provider.plugin_for_command(cmd_name)
245- except errors.NoPluginAvailable:
246- pass
247- else:
248- raise errors.CommandAvailableInPlugin(cmd_name,
249- plugin_metadata, provider)
250- raise KeyError
251+
252+def _get_plugin_command(cmd_or_None, cmd_name):
253+ """Get a command from bzr's plugins."""
254+ try:
255+ return plugin_cmds.get(cmd_name)()
256+ except KeyError:
257+ pass
258+ for key in plugin_cmds.keys():
259+ info = plugin_cmds.get_info(key)
260+ if cmd_name in info.aliases:
261+ return plugin_cmds.get(key)()
262+ return cmd_or_None
263
264
265 class Command(object):
266@@ -608,6 +692,23 @@
267 "Called after creating a command object to allow modifications "
268 "such as adding or removing options, docs etc. Called with the "
269 "new bzrlib.commands.Command object.", (1, 13), None))
270+ self.create_hook(HookPoint('get_command',
271+ "Called when creating a single command. Called with "
272+ "(cmd_or_None, command_name). get_command should either return "
273+ "the cmd_or_None parameter, or a replacement Command object that "
274+ "should be used for the command.", (1, 16), None))
275+ self.create_hook(HookPoint('get_missing_command',
276+ "Called when creating a single command if no command could be "
277+ "found. Called with (command_name). get_missing_command should "
278+ "either return None, or a Command object to be used for the "
279+ "command.", (1, 16), None))
280+ self.create_hook(HookPoint('list_commands',
281+ "Called when enumerating commands. Called with a dict of "
282+ "cmd_name: cmd_class tuples for all the commands found "
283+ "so far. This dict is safe to mutate - to remove a command or "
284+ "to replace it with another (eg plugin supplied) version. "
285+ "list_commands should return the updated dict of commands.",
286+ (1, 16), None))
287
288 Command.hooks = CommandHooks()
289
290@@ -952,6 +1053,20 @@
291 return ignore_pipe
292
293
294+def install_bzr_command_hooks():
295+ """Install the hooks to supply bzr's own commands."""
296+ Command.hooks.install_named_hook("list_commands", _list_bzr_commands,
297+ "bzr commands")
298+ Command.hooks.install_named_hook("get_command", _get_bzr_command,
299+ "bzr commands")
300+ Command.hooks.install_named_hook("get_command", _get_plugin_command,
301+ "bzr plugin commands")
302+ Command.hooks.install_named_hook("get_command", _get_external_command,
303+ "bzr external command lookup")
304+ Command.hooks.install_named_hook("get_missing_command", _try_plugin_provider,
305+ "bzr plugin-provider-db check")
306+
307+
308 def main(argv=None):
309 """Main entry point of command-line interface.
310
311@@ -968,7 +1083,6 @@
312
313 # Is this a final release version? If so, we should suppress warnings
314 if bzrlib.version_info[3] == 'final':
315- from bzrlib import symbol_versioning
316 symbol_versioning.suppress_deprecation_warnings(override=False)
317 if argv is None:
318 argv = osutils.get_unicode_argv()
319@@ -984,6 +1098,7 @@
320 except UnicodeDecodeError:
321 raise errors.BzrError("argv should be list of unicode strings.")
322 argv = new_argv
323+ install_bzr_command_hooks()
324 ret = run_bzr_catch_errors(argv)
325 trace.mutter("return code %d", ret)
326 return ret
327
328=== modified file 'bzrlib/help.py'
329--- bzrlib/help.py 2009-03-23 14:59:43 +0000
330+++ bzrlib/help.py 2009-06-15 08:36:49 +0000
331@@ -73,8 +73,7 @@
332 hidden = True
333 else:
334 hidden = False
335- names = set(_mod_commands.builtin_command_names()) # to eliminate duplicates
336- names.update(_mod_commands.plugin_command_names())
337+ names = list(_mod_commands.all_command_names())
338 commands = ((n, _mod_commands.get_cmd_object(n)) for n in names)
339 shown_commands = [(n, o) for n, o in commands if o.hidden == hidden]
340 max_name = max(len(n) for n, o in shown_commands)
341
342=== modified file 'bzrlib/shellcomplete.py'
343--- bzrlib/shellcomplete.py 2009-03-23 14:59:43 +0000
344+++ bzrlib/shellcomplete.py 2009-06-15 08:36:49 +0000
345@@ -64,15 +64,16 @@
346 outfile = sys.stdout
347
348 cmds = []
349- for cmdname, cmdclass in commands.get_all_cmds():
350- cmds.append((cmdname, cmdclass))
351- for alias in cmdclass.aliases:
352- cmds.append((alias, cmdclass))
353+ for cmdname in commands.all_command_names():
354+ cmd = commands.get_cmd_object(cmdname)))
355+ cmds.append((cmdname, cmd))
356+ for alias in cmd.aliases:
357+ cmds.append((alias, cmd))
358 cmds.sort()
359- for cmdname, cmdclass in cmds:
360- if cmdclass.hidden:
361+ for cmdname, cmd in cmds:
362+ if cmd.hidden:
363 continue
364- doc = getdoc(cmdclass)
365+ doc = getdoc(cmd)
366 if doc is None:
367 outfile.write(cmdname + '\n')
368 else:
369
370=== modified file 'bzrlib/tests/test_commands.py'
371--- bzrlib/tests/test_commands.py 2009-04-07 17:13:51 +0000
372+++ bzrlib/tests/test_commands.py 2009-06-15 08:36:49 +0000
373@@ -163,6 +163,7 @@
374 del sys.modules['bzrlib.tests.fake_command']
375 global lazy_command_imported
376 lazy_command_imported = False
377+ commands.install_bzr_command_hooks()
378
379 @staticmethod
380 def remove_fake():
381@@ -205,6 +206,7 @@
382 # commands are registered).
383 # when they are simply created.
384 hook_calls = []
385+ commands.install_bzr_command_hooks()
386 commands.Command.hooks.install_named_hook(
387 "extend_command", hook_calls.append, None)
388 # create a command, should not fire
389@@ -237,3 +239,87 @@
390 self.assertEqual([cmd], hook_calls)
391 finally:
392 commands.plugin_cmds.remove('fake')
393+
394+
395+class TestGetCommandHook(tests.TestCase):
396+
397+ def test_fires_on_get_cmd_object(self):
398+ # The get_command(cmd) hook fires when commands are delivered to the
399+ # ui.
400+ commands.install_bzr_command_hooks()
401+ hook_calls = []
402+ class ACommand(commands.Command):
403+ """A sample command."""
404+ def get_cmd(cmd_or_None, cmd_name):
405+ hook_calls.append(('called', cmd_or_None, cmd_name))
406+ if cmd_name in ('foo', 'info'):
407+ return ACommand()
408+ commands.Command.hooks.install_named_hook(
409+ "get_command", get_cmd, None)
410+ # create a command directly, should not fire
411+ cmd = ACommand()
412+ self.assertEqual([], hook_calls)
413+ # ask by name, should fire and give us our command
414+ cmd = commands.get_cmd_object('foo')
415+ self.assertEqual([('called', None, 'foo')], hook_calls)
416+ self.assertIsInstance(cmd, ACommand)
417+ del hook_calls[:]
418+ # ask by a name that is supplied by a builtin - the hook should still
419+ # fire and we still get our object, but we should see the builtin
420+ # passed to the hook.
421+ cmd = commands.get_cmd_object('info')
422+ self.assertIsInstance(cmd, ACommand)
423+ self.assertEqual(1, len(hook_calls))
424+ self.assertEqual('info', hook_calls[0][2])
425+ self.assertIsInstance(hook_calls[0][1], builtins.cmd_info)
426+
427+
428+class TestGetMissingCommandHook(tests.TestCase):
429+
430+ def test_fires_on_get_cmd_object(self):
431+ # The get_missing_command(cmd) hook fires when commands are delivered to the
432+ # ui.
433+ hook_calls = []
434+ class ACommand(commands.Command):
435+ """A sample command."""
436+ def get_missing_cmd(cmd_name):
437+ hook_calls.append(('called', cmd_name))
438+ if cmd_name in ('foo', 'info'):
439+ return ACommand()
440+ commands.Command.hooks.install_named_hook(
441+ "get_missing_command", get_missing_cmd, None)
442+ # create a command directly, should not fire
443+ cmd = ACommand()
444+ self.assertEqual([], hook_calls)
445+ # ask by name, should fire and give us our command
446+ cmd = commands.get_cmd_object('foo')
447+ self.assertEqual([('called', 'foo')], hook_calls)
448+ self.assertIsInstance(cmd, ACommand)
449+ del hook_calls[:]
450+ # ask by a name that is supplied by a builtin - the hook should not
451+ # fire and we still get our object.
452+ commands.install_bzr_command_hooks()
453+ cmd = commands.get_cmd_object('info')
454+ self.assertNotEqual(None, cmd)
455+ self.assertEqual(0, len(hook_calls))
456+
457+
458+class TestListCommandHook(tests.TestCase):
459+
460+ def test_fires_on_all_command_names(self):
461+ # The list_commands() hook fires when all_command_names() is invoked.
462+ hook_calls = []
463+ commands.install_bzr_command_hooks()
464+ def list_my_commands(cmd_names):
465+ hook_calls.append('called')
466+ cmd_names.update(['foo', 'bar'])
467+ return cmd_names
468+ commands.Command.hooks.install_named_hook(
469+ "list_commands", list_my_commands, None)
470+ # Get a command, which should not trigger the hook.
471+ cmd = commands.get_cmd_object('info')
472+ self.assertEqual([], hook_calls)
473+ # Get all command classes (for docs and shell completion).
474+ cmds = list(commands.all_command_names())
475+ self.assertEqual(['called'], hook_calls)
476+ self.assertSubset(['foo', 'bar'], cmds)
477
478=== modified file 'bzrlib/tests/test_options.py'
479--- bzrlib/tests/test_options.py 2009-04-03 20:05:25 +0000
480+++ bzrlib/tests/test_options.py 2009-06-15 08:36:49 +0000
481@@ -324,8 +324,8 @@
482
483 def get_builtin_command_options(self):
484 g = []
485- for cmd_name, cmd_class in sorted(commands.get_all_cmds()):
486- cmd = cmd_class()
487+ for cmd_name in sorted(commands.all_command_names()):
488+ cmd = commands.get_cmd_object(cmd_name)
489 for opt_name, opt in sorted(cmd.options().items()):
490 g.append((cmd_name, opt))
491 return g
492@@ -338,14 +338,15 @@
493 g = dict(option.Option.OPTIONS.items())
494 used_globals = {}
495 msgs = []
496- for cmd_name, cmd_class in sorted(commands.get_all_cmds()):
497- for option_or_name in sorted(cmd_class.takes_options):
498+ for cmd_name in sorted(commands.all_command_names()):
499+ cmd = commands.get_cmd_object(cmd_name)
500+ for option_or_name in sorted(cmd.takes_options):
501 if not isinstance(option_or_name, basestring):
502 self.assertIsInstance(option_or_name, option.Option)
503 elif not option_or_name in g:
504 msgs.append("apparent reference to undefined "
505 "global option %r from %r"
506- % (option_or_name, cmd_class))
507+ % (option_or_name, cmd))
508 else:
509 used_globals.setdefault(option_or_name, []).append(cmd_name)
510 unused_globals = set(g.keys()) - set(used_globals.keys())