Code review comment for lp:~spiv/bzr/hooks-refactoring

I would mention get_named_object in the api section of the news, and in the coding standard, so that people do use it in future.

> 2. _LazyObjectGetter(...).get_obj() was buggy,

Is there an actual bug number or user impact for this, or is it just buggy in the sense of being hard to use correctly (which is of course still worth fixing.)

Knowing the modules where all the hook objects can be found seems a bit roundabout to me compared to just actually having the hooks directly in the registry where they can be saved/restored altogether. But this is a cleaner way to implement it.

+known_hooks.register_lazy_hook('bzrlib.branch', 'Branch.hooks', 'BranchHooks')
197 +known_hooks.register_lazy_hook('bzrlib.bzrdir', 'BzrDir.hooks', 'BzrDirHooks')
198 +known_hooks.register_lazy_hook(
199 + 'bzrlib.commands', 'Command.hooks', 'CommandHooks')

For the sake of readability and to avoid all the line wrapping, could you turn this into a

  for (hook_module, hook_attribute_name, hook_class) in _builtin_known_hooks:
    known_hooks.register_lazy_hook(hook_module, hook_attribute_name, hook_class)

+"""Some convenience functions for general Python, such as a wrapper around
+``_import__``.
+"""

Should be a single line.

+def get_named_object(module_name, member_name=None):

It's a pretty generic name. Perhaps the name should include 'import' to give you a bit more of a clue, like import_named_object?

I think this could do with a docstring example (add it to the list of docstrings to test) and it should be feasible to test that way without too much complication. You need to add it to the list anyhow to make the calc_parent_name doctest run.

+ :param module_name: a module name, as found in sys.module. It may contain
+ dots. e.g. 'sys' or 'os.path'.

This seems to imply that it needs to already be in sys.module, but in fact it's fine to call this with a module that might not already be loaded?

Nice removal of duplications there.

tweak