Merge lp:~spiv/bzr/hooks-refactoring into lp:bzr

[Andrew Bennetts]

Add bzrlib/pyutils.py, and replace many awkward (and sometimes buggy) uses of __import__ with calls to bzrlib.pyutils.get_named_object.

While reviewing <https://code.edge.launchpad.net/~parthm/bzr/173274-export-hooks/+merge/35919> I noticed a workaround for a bug in hook collection registration that seemed a bit strange. Digging into it I came to two conclusions:

1. hooks.py was more confusing than necessary
2. _LazyObjectGetter(...).get_obj() was buggy, and that functionality really belonged somewhere more reusable anyway.

This patch definitely addresses 2, but hopefully helps 1 as well.

It adds a new module, bzrlib.pyutils, which has a get_named_object helper for resolving a pair like ('module.submodle', 'attr.another_attr') into the Python object you'd usually access via module.submodule.attr.another_attr (or similar), where the second part is optional and any amount of dots are correctly handled. Importantly, this implementation has *tests*, which means it fixes the bug that _LazyObjectGetter('x.y', None).get_obj() returned x, not x.y. In general if you're doing something more complicated than __import__('top_level_module') with __import__, it's probably easier to use get_named_object instead.

I'm not thrilled by the name of the new module, or the helpers in it. I'm open to suggestions.

The main use of __import__ that I didn't replace is lazy_import.py, partly because it would be a shame to make it less standalone, but mainly because it needs to go to the effort of breaking the import statements down into "import this module, get a reference to module, get this attribute from that module" steps anyway, because it's parsing actual "import" statements in all their varied forms... so get_named_object isn't a drop-in replacement for its current logic.

One thing get_named_object does slightly differently than many __import__ calls it replaces is resolving a module name of 'x.y.z' by importing x.y.z, then fetching it from sys.modules['x.y.z'] directly. Previously we tended to do the __import__, which would return 'x', and then repeatedly getattr to get to z (or abuse __import__ by passing a fake from_list). Both ways should be equivalent barring really evil code, and the Python docs recommend the sys.modules method, and it should be marginally more efficient I think, just in case it matters ;)

[Martin Pool (mbp)]

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

review: Needs Fixing

[lp:~spiv/bzr/hooks-refactoring]

5438. By Andrew Bennetts on 2010-09-28

Merge lp:bzr.

5439. By Andrew Bennetts on 2010-09-28

Add NEWS entry.

5440. By Andrew Bennetts on 2010-09-28

Add section to code-style.txt recommending get_named_object.

5441. By Andrew Bennetts on 2010-09-28

Tweak builtin known_hooks registration per poolie's review.

5442. By Andrew Bennetts on 2010-09-28

Add doctest-able example to get_named_object docstring. Make doctest skip calc_parent_name docstring.

5443. By Andrew Bennetts on 2010-09-28

Docstring tweaks prompted by review.

[Andrew Bennetts (spiv)]

Martin Pool wrote:
> Review: Needs Fixing
> 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.

Done.

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

No bug number I found, but it came up during a review (of a currently back in
work-in-progress) patch to add pre- and post-export hooks. There was a strange
workaround in the hook registration to do with some sort of interaction with the
hook point being a module-global in a __init__.py, rather than an attribute of
an object.

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

Hmm, quite possibly. I don't have any incentive to worry about that atm, though
:)

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

Ok, done, although a couple of them still needed to be wrapped.

> +"""Some convenience functions for general Python, such as a wrapper around
> +``_import__``.
> +"""
>
> Should be a single line.

Really? I don't think you can mean those three lines should be on one, because
that's over 80 columns.

I guess you mean the closing triple-double-quotes should be on the same line as
the final line of the text. I interpret this situation as a multiline
docstring, where AIUI the convention is closing quotes get their own line. I
guess you consider the fact it's one sentence to mean it's more like a single
line docstring?

Anyway, changed, because while I disagree I also don't care very much :)

If it really bugs us too much just add another paragraph to that docstring to
make the situation unambiguous... ;)

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

It is pretty generic :(, but it seems other projects solving this problem have
chosen similarly generic names, so perhaps it's unavoidable. There's some
overlap with twisted.python.util.getNamedAny, and I saw today that unittest2 has
getObjectFromName which is also similar in purpose. So it appears the broader
Python community tends towards this sort of generic name (and this feature
should be part of core Python, clearly...)

I think “import” is perhaps a misleading hint, because that's only part of what
it does.

> I think this could do with a docstring example (add it to the...

[Martin Pool (mbp)]

On 28 September 2010 18:37, Andrew Bennetts
<email address hidden> wrote:
>> +"""Some convenience functions for general Python, such as a wrapper around
>> +``_import__``.
>> +"""
>>
>> Should be a single line.
>
> Really?  I don't think you can mean those three lines should be on one, because
> that's over 80 columns.
>
> I guess you mean the closing triple-double-quotes should be on the same line as
> the final line of the text.  I interpret this situation as a multiline
> docstring, where AIUI the convention is closing quotes get their own line.  I
> guess you consider the fact it's one sentence to mean it's more like a single
> line docstring?

No, I meant that I thought the Python convention was that docstrings
should be a single sentence that fits on a single line, such as

"""General Python convenience functions.
"""

Perhaps it doesn't matter if it's on one line and it just needs to be
a single-sentence paragraph?

>
> Anyway, changed, because while I disagree I also don't care very much :)
>
> If it really bugs us too much just add another paragraph to that docstring to
> make the situation unambiguous... ;)
>
>> +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?
>
> It is pretty generic :(, but it seems other projects solving this problem have
> chosen similarly generic names, so perhaps it's unavoidable.  There's some
> overlap with twisted.python.util.getNamedAny, and I saw today that unittest2 has
> getObjectFromName which is also similar in purpose.  So it appears the broader
> Python community tends towards this sort of generic name (and this feature
> should be part of core Python, clearly...)
>
> I think “import” is perhaps a misleading hint, because that's only part of what
> it does.

ok

>
>> 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.
>
> I've added a doctest-friendly example to get_named_object, and added to the list
> of doctested modules.
>
> The calc_parent_name example is not a doctest, though.  It's simply
> documentation, because the effort to contrive a doctest-runnable example is a
> bit disproportionate (and so I think would detract from the clarity of the
> example as documentation).  So it fails when I add pyutils to the list of
> doctestable modules, and I have to sprinkle #doctest: +SKIP to avoid that.  The
> docstring example is pretty much verbatim how I expect the actual callers of the
> code to look.
>
>> +    :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?
>
> Updated to:
>
>    :param module_name: a module name, as would be found in sys.modules if
>        the module is already imported.  It may contain dots.  e.g. 'sys' or
> ...

[Robert Collins (lifeless)]

On Wed, Sep 29, 2010 at 3:43 PM, Martin Pool <email address hidden> wrote:
> No, I meant that I thought the Python convention was that docstrings
> should be a single sentence that fits on a single line, such as
>
> """General Python convenience functions.
> """

I may be misunderstanding, but that particular layout really squicks.

"""General Python convenience functions."""

or
"""General Python convenience functions.

More detail here.
"""

> Perhaps it doesn't matter if it's on one line and it just needs to be
> a single-sentence paragraph?

http://www.python.org/dev/peps/pep-0257/
"The closing quotes are on the same line as the opening quotes. This
looks better for one-liners."

etc.

_Rob

[Martin Pool (mbp)]

I don't personally care very much where the quotes are, but what I was
asking for, was, from pep 257

>> Multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description. The summary line may be used by automatic indexing tools; it is important that it fits on one line and is separated from the rest of the docstring by a blank line.

If you have eight characters of indent plus six characters of quotes
and a limit around 72-80 you do need to come up with a pretty terse
(down to 58 chars) summary sentence.

--
Martin

[Robert Collins (lifeless)]

On Wed, Sep 29, 2010 at 4:25 PM, Martin Pool <email address hidden> wrote:
>>> Multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description. The summary line may be used by automatic indexing tools; it is important that it fits on one line and is separated from the rest of the docstring by a blank line.
>
> If you have eight characters of indent plus six characters of quotes
> and a limit around 72-80 you do need to come up with a pretty terse
> (down to 58 chars) summary sentence.

Yea, it can be quite Zen sometimes. I wonder if trying for koans would help.

-Rob

[Andrew Bennetts (spiv)]

Martin Pool wrote:
> On 28 September 2010 18:37, Andrew Bennetts
> <email address hidden> wrote:
> >> +"""Some convenience functions for general Python, such as a wrapper around
> >> +``_import__``.
> >> +"""
> >>
> >> Should be a single line.
> >
> > Really?  I don't think you can mean those three lines should be on one, because
> > that's over 80 columns.
> >
> > I guess you mean the closing triple-double-quotes should be on the same line as
> > the final line of the text.  I interpret this situation as a multiline
> > docstring, where AIUI the convention is closing quotes get their own line.  I
> > guess you consider the fact it's one sentence to mean it's more like a single
> > line docstring?
>
> No, I meant that I thought the Python convention was that docstrings
> should be a single sentence that fits on a single line, such as
>
> """General Python convenience functions.
> """
>
> Perhaps it doesn't matter if it's on one line and it just needs to be
> a single-sentence paragraph?

In this instance, your proposed one-liner is good enough for me, so let's do
that.

In general, I don't bend over backwards to make the summary sentence fit on one
line: sometimes things really are a bit complex and a short, vague “does stuff”
that forces you to read the body seems to do the reader more of a disservice a
slightly-more-than-one-line initial sentence.

It's probably worth checking what pydoc and epydoc do with these overrunning
sentences... As a core developer I'm primarily reading them in the
source directly, but that might not be true for authors of code using bzrlib.

-Andrew.

[Martin Pool (mbp)]

On 5 October 2010 10:55, Andrew Bennetts <email address hidden> wrote:
> In this instance, your proposed one-liner is good enough for me, so let's do
> that.
>
> In general, I don't bend over backwards to make the summary sentence fit on one
> line: sometimes things really are a bit complex and a short, vague “does stuff”
> that forces you to read the body seems to do the reader more of a disservice a
> slightly-more-than-one-line initial sentence.
>
> It's probably worth checking what pydoc and epydoc do with these overrunning
> sentences...  As a core developer I'm primarily reading them in the
> source directly, but that might not be true for authors of code using bzrlib.

I agree with all that. I stick to it in the unverifiied belief it
might prevent epydoc printing truncated sentences as summaries. It's
not a big deal, just something I noticed.

--
Martin

[Vincent Ladeuil (vila)]

Are there still bits pending review here ?
Or is there a consensus already ?

[Vincent Ladeuil (vila)]

In other words: do you need help from patch pilot or can this be landed ?

review: Needs Information

[Vincent Ladeuil (vila)]

Ping.

https://code.edge.launchpad.net/~gz/bzr/transport_post_connect_hook/+merge/38431 is now waiting for https://code.edge.launchpad.net/~parthm/bzr/173274-export-hooks which, AIUI, is waiting on this mp.

=== modified file 'NEWS'
--- NEWS	2010-09-27 19:31:45 +0000
+++ NEWS	2010-09-28 08:31:43 +0000
@@ -8,7 +8,7 @@
 bzr 2.3b2
 #########
 
-:2.3.b2: NOT RELEASED YET
+:2.3b2: NOT RELEASED YET
 
 Compatibility Breaks
 ********************
@@ -45,6 +45,14 @@
 API Changes
 ***********
 
+* Add ``bzrlib.pyutils`` module with helper functions for some Python
+  tasks such as resolving a dotted name to a Python object
+  (``get_named_object``).  (Andrew Bennetts)
+
+* ``known_hooks_key_to_parent_and_attribute`` in ``bzrlib.hooks`` has been
+  deprecated in favour of ``known_hooks.key_to_parent_and_attribute`` in
+  the same module.  (Andrew Bennetts)
+
 Internals
 *********
 

=== modified file 'bzrlib/bundle/serializer/__init__.py'
--- bzrlib/bundle/serializer/__init__.py	2010-07-15 12:53:00 +0000
+++ bzrlib/bundle/serializer/__init__.py	2010-09-28 08:31:43 +0000
@@ -21,7 +21,10 @@
 from StringIO import StringIO
 import re
 
-import bzrlib.errors as errors
+from bzrlib import (
+    errors,
+    pyutils,
+    )
 from bzrlib.diff import internal_diff
 from bzrlib.revision import NULL_REVISION
 # For backwards-compatibility
@@ -191,8 +194,7 @@
     :param overwrite: Should this version override a default
     """
     def _loader(version):
-        mod = __import__(module, globals(), locals(), [classname])
-        klass = getattr(mod, classname)
+        klass = pyutils.get_named_object(module, classname)
         return klass(version)
     register(version, _loader, overwrite=overwrite)
 

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2010-09-10 09:46:15 +0000
+++ bzrlib/bzrdir.py	2010-09-28 08:31:43 +0000
@@ -45,6 +45,7 @@
     lockable_files,
     lockdir,
     osutils,
+    pyutils,
     remote,
     repository,
     revision as _mod_revision,
@@ -3092,15 +3093,12 @@
     def _load(full_name):
         mod_name, factory_name = full_name.rsplit('.', 1)
         try:
-            mod = __import__(mod_name, globals(), locals(),
-                    [factory_name])
+            factory = pyutils.get_named_object(mod_name, factory_name)
         except ImportError, e:
             raise ImportError('failed to load %s: %s' % (full_name, e))
-        try:
-            factory = getattr(mod, factory_name)
         except AttributeError:
             raise AttributeError('no factory %s in module %r'
-                % (full_name, mod))
+                % (full_name, sys.modules[mod_name]))
         return factory()
 
     def helper():

=== modified file 'bzrlib/export/__init__.py'
--- bzrlib/export/__init__.py	2010-03-25 09:39:03 +0000
+++ bzrlib/export/__init__.py	2010-09-28 08:31:43 +0000
@@ -20,7 +20,10 @@
 """
 
 import os
-import bzrlib.errors as errors
+from bzrlib import (
+    errors,
+    pyutils,
+    )
 
 # Maps format name => export function
 _exporters = {}
@@ -55,8 +58,7 @@
     When requesting a specific type of export, load the respective path.
     """
     def _loader(tree, dest, root, subdir, filtered, per_file_timestamps):
-        mod = __import__(module, globals(), locals(), [funcname])
-        func = getattr(mod, funcname)
+        func = pyutils.get_named_object(module, funcname)
         return func(tree, dest, root, subdir, filtered=filtered,
                     per_file_timestamps=per_file_timestamps)
     register_exporter(scheme, extensions, _loader)

=== modified file 'bzrlib/hooks.py'
--- bzrlib/hooks.py	2010-08-28 06:13:48 +0000
+++ bzrlib/hooks.py	2010-09-28 08:31:43 +0000
@@ -16,7 +16,11 @@
 
 
 """Support for plugin hooking logic."""
-from bzrlib import registry
+from bzrlib import (
+    pyutils,
+    registry,
+    symbol_versioning,
+    )
 from bzrlib.lazy_import import lazy_import
 lazy_import(globals(), """
 import textwrap
@@ -29,39 +33,63 @@
 """)
 
 
-known_hooks = registry.Registry()
-# known_hooks registry contains
-# tuple of (module, member name) which is the hook point
-# module where the specific hooks are defined
-# callable to get the empty specific Hooks for that attribute
-known_hooks.register_lazy(('bzrlib.branch', 'Branch.hooks'), 'bzrlib.branch',
-    'BranchHooks')
-known_hooks.register_lazy(('bzrlib.bzrdir', 'BzrDir.hooks'), 'bzrlib.bzrdir',
-    'BzrDirHooks')
-known_hooks.register_lazy(('bzrlib.commands', 'Command.hooks'),
-    'bzrlib.commands', 'CommandHooks')
-known_hooks.register_lazy(('bzrlib.info', 'hooks'),
-    'bzrlib.info', 'InfoHooks')
-known_hooks.register_lazy(('bzrlib.lock', 'Lock.hooks'), 'bzrlib.lock',
-    'LockHooks')
-known_hooks.register_lazy(('bzrlib.merge', 'Merger.hooks'), 'bzrlib.merge',
-    'MergeHooks')
-known_hooks.register_lazy(('bzrlib.msgeditor', 'hooks'), 'bzrlib.msgeditor',
-    'MessageEditorHooks')
-known_hooks.register_lazy(('bzrlib.mutabletree', 'MutableTree.hooks'),
-    'bzrlib.mutabletree', 'MutableTreeHooks')
-known_hooks.register_lazy(('bzrlib.smart.client', '_SmartClient.hooks'),
-    'bzrlib.smart.client', 'SmartClientHooks')
-known_hooks.register_lazy(('bzrlib.smart.server', 'SmartTCPServer.hooks'),
-    'bzrlib.smart.server', 'SmartServerHooks')
-known_hooks.register_lazy(('bzrlib.status', 'hooks'),
-    'bzrlib.status', 'StatusHooks')
-known_hooks.register_lazy(
-    ('bzrlib.version_info_formats.format_rio', 'RioVersionInfoBuilder.hooks'),
-    'bzrlib.version_info_formats.format_rio', 'RioVersionInfoBuilderHooks')
-known_hooks.register_lazy(
-    ('bzrlib.merge_directive', 'BaseMergeDirective.hooks'),
-    'bzrlib.merge_directive', 'MergeDirectiveHooks')
+class KnownHooksRegistry(registry.Registry):
+    # known_hooks registry contains
+    # tuple of (module, member name) which is the hook point
+    # module where the specific hooks are defined
+    # callable to get the empty specific Hooks for that attribute
+
+    def register_lazy_hook(self, hook_module_name, hook_member_name,
+            hook_factory_member_name):
+        self.register_lazy((hook_module_name, hook_member_name),
+            hook_module_name, hook_factory_member_name)
+
+    def iter_parent_objects(self):
+        """Yield (hook_key, (parent_object, attr)) tuples for every registered
+        hook, where 'parent_object' is the object that holds the hook
+        instance.
+
+        This is useful for resetting/restoring all the hooks to a known state,
+        as is done in bzrlib.tests.TestCase._clear_hooks.
+        """
+        for key in self.keys():
+            yield key, self.key_to_parent_and_attribute(key)
+
+    def key_to_parent_and_attribute(self, (module_name, member_name)):
+        """Convert a known_hooks key to a (parent_obj, attr) pair.
+
+        :param key: A tuple (module_name, member_name) as found in the keys of
+            the known_hooks registry.
+        :return: The parent_object of the hook and the name of the attribute on
+            that parent object where the hook is kept.
+        """
+        parent_mod, parent_member, attr = pyutils.calc_parent_name(module_name,
+            member_name)
+        return pyutils.get_named_object(parent_mod, parent_member), attr
+
+
+_builtin_known_hooks = (
+    ('bzrlib.branch', 'Branch.hooks', 'BranchHooks'),
+    ('bzrlib.bzrdir', 'BzrDir.hooks', 'BzrDirHooks'),
+    ('bzrlib.commands', 'Command.hooks', 'CommandHooks'),
+    ('bzrlib.info', 'hooks', 'InfoHooks'),
+    ('bzrlib.lock', 'Lock.hooks', 'LockHooks'),
+    ('bzrlib.merge', 'Merger.hooks', 'MergeHooks'),
+    ('bzrlib.msgeditor', 'hooks', 'MessageEditorHooks'),
+    ('bzrlib.mutabletree', 'MutableTree.hooks', 'MutableTreeHooks'),
+    ('bzrlib.smart.client', '_SmartClient.hooks', 'SmartClientHooks'),
+    ('bzrlib.smart.server', 'SmartTCPServer.hooks', 'SmartServerHooks'),
+    ('bzrlib.status', 'hooks', 'StatusHooks'),
+    ('bzrlib.version_info_formats.format_rio', 'RioVersionInfoBuilder.hooks',
+        'RioVersionInfoBuilderHooks'),
+    ('bzrlib.merge_directive', 'BaseMergeDirective.hooks',
+        'MergeDirectiveHooks'),
+    )
+
+known_hooks = KnownHooksRegistry()
+for (_hook_module, _hook_attribute, _hook_class) in _builtin_known_hooks:
+    known_hooks.register_lazy_hook(_hook_module, _hook_attribute, _hook_class)
+del _builtin_known_hooks, _hook_module, _hook_attribute, _hook_class
 
 
 def known_hooks_key_to_object((module_name, member_name)):
@@ -71,24 +99,13 @@
         the known_hooks registry.
     :return: The object this specifies.
     """
-    return registry._LazyObjectGetter(module_name, member_name).get_obj()
-
-
-def known_hooks_key_to_parent_and_attribute((module_name, member_name)):
-    """Convert a known_hooks key to a object.
-
-    :param key: A tuple (module_name, member_name) as found in the keys of
-        the known_hooks registry.
-    :return: The object this specifies.
-    """
-    member_list = member_name.rsplit('.', 1)
-    if len(member_list) == 2:
-        parent_name, attribute = member_list
-    else:
-        parent_name = None
-        attribute = member_name
-    parent = known_hooks_key_to_object((module_name, parent_name))
-    return parent, attribute
+    return pyutils.get_named_object(module_name, member_name)
+
+
+@symbol_versioning.deprecated_function(symbol_versioning.deprecated_in((2, 3)))
+def known_hooks_key_to_parent_and_attribute(key):
+    """See KnownHooksRegistry.key_to_parent_and_attribute."""
+    return known_hooks.key_to_parent_and_attribute(key)
 
 
 class Hooks(dict):

=== added file 'bzrlib/pyutils.py'
--- bzrlib/pyutils.py	1970-01-01 00:00:00 +0000
+++ bzrlib/pyutils.py	2010-09-28 08:31:43 +0000
@@ -0,0 +1,91 @@
+# Copyright (C) 2010 Canonical Ltd
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+"""Some convenience functions for general Python, such as a wrapper around
+``_import__``."""
+
+
+import sys
+
+
+def get_named_object(module_name, member_name=None):
+    """Get the Python object named by a given module and member name.
+
+    This is usually much more convenient than dealing with ``__import__``
+    directly::
+
+        >>> doc = get_named_object('bzrlib.pyutils', 'get_named_object.__doc__')
+        >>> doc.splitlines()[0]
+        'Get the Python object named by a given module and member name.'
+
+    :param module_name: a module name, as would be found in sys.modules if
+        the module is already imported.  It may contain dots.  e.g. 'sys' or
+        'os.path'.
+    :param member_name: (optional) a name of an attribute in that module to
+        return.  It may contain dots.  e.g. 'MyClass.some_method'.  If not
+        given, the named module will be returned instead.
+    :raises: ImportError or AttributeError.
+    """
+    # We may have just a module name, or a module name and a member name,
+    # and either may contain dots.  __import__'s return value is a bit
+    # unintuitive, so we need to take care to always return the object
+    # specified by the full combination of module name + member name.
+    if member_name:
+        # Give __import__ a from_list.  It will return the last module in
+        # the dotted module name.
+        attr_chain = member_name.split('.')
+        from_list = attr_chain[:1]
+        obj = __import__(module_name, {}, {}, from_list)
+        for attr in attr_chain:
+            obj = getattr(obj, attr)
+    else:
+        # We're just importing a module, no attributes, so we have no
+        # from_list.  __import__ will return the first module in the dotted
+        # module name, so we look up the module from sys.modules.
+        __import__(module_name, globals(), locals(), [])
+        obj = sys.modules[module_name]
+    return obj
+
+
+def calc_parent_name(module_name, member_name=None):
+    """Determine the 'parent' of a given dotted module name and (optional)
+    member name.
+
+    Typical use is::
+
+        >>> parent_mod, parent_member, final_attr = calc_parent_name(
+        ...     module_name, member_name) # doctest: +SKIP
+        >>> parent_obj = get_named_object(parent_mod, parent_member)
+        ... # doctest: +SKIP
+
+    The idea is that ``getattr(parent_obj, final_attr)`` will equal
+    get_named_object(module_name, member_name).
+
+    :return: (module_name, member_name, final_attr) tuple.
+    """
+    if member_name is not None:
+        split_name = member_name.rsplit('.', 1)
+        if len(split_name) == 1:
+            return (module_name, None, member_name)
+        else:
+            return (module_name, split_name[0], split_name[1])
+    else:
+        split_name = module_name.rsplit('.', 1)
+        if len(split_name) == 1:
+            raise AssertionError(
+                'No parent object for top-level module %r' % (module_name,))
+        else:
+            return (split_name[0], None, split_name[1])

=== modified file 'bzrlib/registry.py'
--- bzrlib/registry.py	2009-09-16 10:37:29 +0000
+++ bzrlib/registry.py	2010-09-28 08:31:43 +0000
@@ -17,6 +17,9 @@
 """Classes to provide name-to-object registry-like support."""
 
 
+from bzrlib.pyutils import get_named_object
+
+
 class _ObjectGetter(object):
     """Maintain a reference to an object, and return the object on request.
 
@@ -58,26 +61,14 @@
         return the imported object.
         """
         if not self._imported:
-            self._do_import()
+            self._obj = get_named_object(self._module_name, self._member_name)
+            self._imported = True
         return super(_LazyObjectGetter, self).get_obj()
 
-    def _do_import(self):
-        if self._member_name:
-            segments = self._member_name.split('.')
-            names = segments[0:1]
-        else:
-            names = [self._member_name]
-        obj = __import__(self._module_name, globals(), locals(), names)
-        if self._member_name:
-            for segment in segments:
-                obj = getattr(obj, segment)
-        self._obj = obj
-        self._imported = True
-
     def __repr__(self):
-        return "<%s.%s object at %x, module=%r attribute=%r>" % (
+        return "<%s.%s object at %x, module=%r attribute=%r imported=%r>" % (
             self.__class__.__module__, self.__class__.__name__, id(self),
-            self._module_name, self._member_name)
+            self._module_name, self._member_name, self._imported)
 
 
 class Registry(object):

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py	2010-09-24 01:59:46 +0000
+++ bzrlib/repository.py	2010-09-28 08:31:43 +0000
@@ -39,6 +39,7 @@
     lockdir,
     lru_cache,
     osutils,
+    pyutils,
     revision as _mod_revision,
     static_tuple,
     symbol_versioning,
@@ -52,6 +53,7 @@
 from bzrlib.testament import Testament
 """)
 
+import sys
 from bzrlib import (
     errors,
     registry,
@@ -2825,12 +2827,11 @@
             % (name, from_module),
             DeprecationWarning,
             stacklevel=2)
-        m = __import__(from_module, globals(), locals(), [name])
         try:
-            return getattr(m, name)
+            return pyutils.get_named_object(from_module, name)
         except AttributeError:
             raise AttributeError('module %s has no name %s'
-                    % (m, name))
+                    % (sys.modules[from_module], name))
     globals()[name] = _deprecated_repository_forwarder
 
 for _name in [

=== modified file 'bzrlib/tests/TestUtil.py'
--- bzrlib/tests/TestUtil.py	2010-08-05 18:13:23 +0000
+++ bzrlib/tests/TestUtil.py	2010-09-28 08:31:43 +0000
@@ -20,6 +20,8 @@
 import logging
 import unittest
 
+from bzrlib import pyutils
+
 # Mark this python module as being part of the implementation
 # of unittest: this gives us better tracebacks where the last
 # shown frame is the test code, not our assertXYZ.
@@ -106,7 +108,7 @@
 
     def loadTestsFromModuleName(self, name):
         result = self.suiteClass()
-        module = _load_module_by_name(name)
+        module = pyutils.get_named_object(name)
 
         result.addTests(self.loadTestsFromModule(module))
         return result
@@ -179,17 +181,6 @@
             return self.suiteClass()
 
 
-def _load_module_by_name(mod_name):
-    parts = mod_name.split('.')
-    module = __import__(mod_name)
-    del parts[0]
-    # for historical reasons python returns the top-level module even though
-    # it loads the submodule; we need to walk down to get the one we want.
-    while parts:
-        module = getattr(module, parts.pop(0))
-    return module
-
-
 class TestVisitor(object):
     """A visitor for Tests"""
     def visitSuite(self, aTestSuite):

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2010-09-27 19:31:45 +0000
+++ bzrlib/tests/__init__.py	2010-09-28 08:31:43 +0000
@@ -71,6 +71,7 @@
     lock as _mod_lock,
     memorytree,
     osutils,
+    pyutils,
     ui,
     urlutils,
     registry,
@@ -877,14 +878,14 @@
 
     def _clear_hooks(self):
         # prevent hooks affecting tests
+        known_hooks = hooks.known_hooks
         self._preserved_hooks = {}
-        for key, factory in hooks.known_hooks.items():
-            parent, name = hooks.known_hooks_key_to_parent_and_attribute(key)
-            current_hooks = hooks.known_hooks_key_to_object(key)
+        for key, (parent, name) in known_hooks.iter_parent_objects():
+            current_hooks = getattr(parent, name)
             self._preserved_hooks[parent] = (name, current_hooks)
         self.addCleanup(self._restoreHooks)
-        for key, factory in hooks.known_hooks.items():
-            parent, name = hooks.known_hooks_key_to_parent_and_attribute(key)
+        for key, (parent, name) in known_hooks.iter_parent_objects():
+            factory = known_hooks.get(key)
             setattr(parent, name, factory())
         # this hook should always be installed
         request._install_hook()
@@ -3773,6 +3774,7 @@
         'bzrlib.tests.test_permissions',
         'bzrlib.tests.test_plugins',
         'bzrlib.tests.test_progress',
+        'bzrlib.tests.test_pyutils',
         'bzrlib.tests.test_read_bundle',
         'bzrlib.tests.test_reconcile',
         'bzrlib.tests.test_reconfigure',
@@ -3856,6 +3858,7 @@
         'bzrlib.lockdir',
         'bzrlib.merge3',
         'bzrlib.option',
+        'bzrlib.pyutils',
         'bzrlib.symbol_versioning',
         'bzrlib.tests',
         'bzrlib.tests.fixtures',
@@ -4098,7 +4101,7 @@
         the module is available.
     """
 
-    py_module = __import__(py_module_name, {}, {}, ['NO_SUCH_ATTRIB'])
+    py_module = pyutils.get_named_object(py_module_name)
     scenarios = [
         ('python', {'module': py_module}),
     ]
@@ -4257,9 +4260,8 @@
             symbol_versioning.warn(depr_msg + use_msg, DeprecationWarning)
             # Import the new feature and use it as a replacement for the
             # deprecated one.
-            mod = __import__(self._replacement_module, {}, {},
-                             [self._replacement_name])
-            self._feature = getattr(mod, self._replacement_name)
+            self._feature = pyutils.get_named_object(
+                self._replacement_module, self._replacement_name)
 
     def _probe(self):
         self._ensure()

=== modified file 'bzrlib/tests/per_transport.py'
--- bzrlib/tests/per_transport.py	2010-08-24 13:03:18 +0000
+++ bzrlib/tests/per_transport.py	2010-09-28 08:31:43 +0000
@@ -26,28 +26,24 @@
 from StringIO import StringIO as pyStringIO
 import stat
 import sys
-import unittest
 
 from bzrlib import (
     errors,
     osutils,
+    pyutils,
     tests,
     urlutils,
     )
 from bzrlib.errors import (ConnectionError,
-                           DirectoryNotEmpty,
                            FileExists,
                            InvalidURL,
-                           LockError,
                            NoSuchFile,
-                           NotLocalUrl,
                            PathError,
                            TransportNotPossible,
                            )
 from bzrlib.osutils import getcwd
 from bzrlib.smart import medium
 from bzrlib.tests import (
-    TestCaseInTempDir,
     TestSkipped,
     TestNotApplicable,
     multiply_tests,
@@ -78,7 +74,7 @@
     for module in _get_transport_modules():
         try:
             permutations = get_transport_test_permutations(
-                reduce(getattr, (module).split('.')[1:], __import__(module)))
+                pyutils.get_named_object(module))
             for (klass, server_factory) in permutations:
                 scenario = ('%s,%s' % (klass.__name__, server_factory.__name__),
                     {"transport_class":klass,

=== modified file 'bzrlib/tests/test_hooks.py'
--- bzrlib/tests/test_hooks.py	2010-02-17 17:11:16 +0000
+++ bzrlib/tests/test_hooks.py	2010-09-28 08:31:43 +0000
@@ -28,6 +28,9 @@
     known_hooks_key_to_object,
     known_hooks_key_to_parent_and_attribute,
     )
+from bzrlib.symbol_versioning import (
+    deprecated_in,
+    )
 
 
 class TestHooks(tests.TestCase):
@@ -175,10 +178,20 @@
         self.assertIs(branch.Branch.hooks,
             known_hooks_key_to_object(('bzrlib.branch', 'Branch.hooks')))
 
+    def test_known_hooks_key_to_parent_and_attribute_deprecated(self):
+        self.assertEqual((branch.Branch, 'hooks'),
+            self.applyDeprecated(deprecated_in((2,3)),
+                known_hooks_key_to_parent_and_attribute,
+                ('bzrlib.branch', 'Branch.hooks')))
+        self.assertEqual((branch, 'Branch'),
+            self.applyDeprecated(deprecated_in((2,3)),
+                known_hooks_key_to_parent_and_attribute,
+                ('bzrlib.branch', 'Branch')))
+
     def test_known_hooks_key_to_parent_and_attribute(self):
         self.assertEqual((branch.Branch, 'hooks'),
-            known_hooks_key_to_parent_and_attribute(
+            known_hooks.key_to_parent_and_attribute(
             ('bzrlib.branch', 'Branch.hooks')))
         self.assertEqual((branch, 'Branch'),
-            known_hooks_key_to_parent_and_attribute(
+            known_hooks.key_to_parent_and_attribute(
             ('bzrlib.branch', 'Branch')))

=== added file 'bzrlib/tests/test_pyutils.py'
--- bzrlib/tests/test_pyutils.py	1970-01-01 00:00:00 +0000
+++ bzrlib/tests/test_pyutils.py	2010-09-28 08:31:43 +0000
@@ -0,0 +1,88 @@
+# Copyright (C) 2010 Canonical Ltd
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+"""Tests for bzrlib.pyutils."""
+
+from bzrlib import (
+    branch,
+    tests,
+    )
+from bzrlib.pyutils import (
+    calc_parent_name,
+    get_named_object,
+    )
+
+
+class TestGetNamedObject(tests.TestCase):
+    """Tests for get_named_object."""
+
+    def test_module_only(self):
+        import sys
+        self.assertIs(sys, get_named_object('sys'))
+
+    def test_dotted_module(self):
+        self.assertIs(branch, get_named_object('bzrlib.branch'))
+
+    def test_module_attr(self):
+        self.assertIs(
+            branch.Branch, get_named_object('bzrlib.branch', 'Branch'))
+
+    def test_dotted_attr(self):
+        self.assertIs(
+            branch.Branch.hooks,
+            get_named_object('bzrlib.branch', 'Branch.hooks'))
+
+    def test_package(self):
+        # bzrlib.tests is a package, not simply a module
+        self.assertIs(tests, get_named_object('bzrlib.tests'))
+
+    def test_package_attr(self):
+        # bzrlib.tests is a package, not simply a module
+        self.assertIs(
+            tests.TestCase, get_named_object('bzrlib.tests', 'TestCase'))
+
+    def test_import_error(self):
+        self.assertRaises(ImportError, get_named_object, 'NO_SUCH_MODULE')
+
+    def test_attribute_error(self):
+        self.assertRaises(
+            AttributeError, get_named_object, 'sys', 'NO_SUCH_ATTR')
+
+
+
+class TestCalcParent_name(tests.TestCase):
+    """Tests for calc_parent_name."""
+
+    def test_dotted_member(self):
+        self.assertEqual(
+            ('mod_name', 'attr1', 'attr2'),
+            calc_parent_name('mod_name', 'attr1.attr2'))
+
+    def test_undotted_member(self):
+        self.assertEqual(
+            ('mod_name', None, 'attr1'),
+            calc_parent_name('mod_name', 'attr1'))
+
+    def test_dotted_module_no_member(self):
+        self.assertEqual(
+            ('mod', None, 'sub_mod'),
+            calc_parent_name('mod.sub_mod'))
+
+    def test_undotted_module_no_member(self):
+        err = self.assertRaises(AssertionError, calc_parent_name, 'mod_name')
+        self.assertEqual(
+            "No parent object for top-level module 'mod_name'", err.args[0])
+

=== modified file 'bzrlib/tests/test_registry.py'
--- bzrlib/tests/test_registry.py	2009-09-16 10:37:29 +0000
+++ bzrlib/tests/test_registry.py	2010-09-28 08:31:43 +0000
@@ -20,6 +20,7 @@
 import sys
 
 from bzrlib import (
+    branch,
     errors,
     osutils,
     registry,
@@ -286,6 +287,13 @@
             '\n\n'
         )
 
+    def test_lazy_import_registry_foo(self):
+        a_registry = registry.Registry()
+        a_registry.register_lazy('foo', 'bzrlib.branch', 'Branch')
+        a_registry.register_lazy('bar', 'bzrlib.branch', 'Branch.hooks')
+        self.assertEqual(branch.Branch, a_registry.get('foo'))
+        self.assertEqual(branch.Branch.hooks, a_registry.get('bar'))
+
     def test_lazy_import_registry(self):
         plugin_name = self.create_simple_plugin()
         a_registry = registry.Registry()

=== modified file 'bzrlib/tests/test_selftest.py'
--- bzrlib/tests/test_selftest.py	2010-09-27 19:31:45 +0000
+++ bzrlib/tests/test_selftest.py	2010-09-28 08:31:43 +0000
@@ -78,18 +78,6 @@
     return [t.id() for t in tests.iter_suite_tests(test_suite)]
 
 
-class SelftestTests(tests.TestCase):
-
-    def test_import_tests(self):
-        mod = TestUtil._load_module_by_name('bzrlib.tests.test_selftest')
-        self.assertEqual(mod.SelftestTests, SelftestTests)
-
-    def test_import_test_failure(self):
-        self.assertRaises(ImportError,
-                          TestUtil._load_module_by_name,
-                          'bzrlib.no-name-yet')
-
-
 class MetaTestLog(tests.TestCase):
 
     def test_logging(self):

=== modified file 'doc/developers/code-style.txt'
--- doc/developers/code-style.txt	2010-09-24 08:42:02 +0000
+++ doc/developers/code-style.txt	2010-09-28 08:31:43 +0000
@@ -485,5 +485,19 @@
 
  * Don't say "open source" when you mean "free software".
 
+
+Dynamic imports
+===============
+
+If you need to import a module (or attribute of a module) named in a
+variable:
+
+ * If importing a module, not an attribute, and the module is a top-level
+   module (i.e. has no dots in the name), then it's ok to use the builtin
+   ``__import__``, e.g. ``__import__(module_name)``.
+ * In all other cases, prefer ``bzrlib.pyutils.get_named_object`` to the
+   built-in ``__import__``.  ``__import__`` has some subtleties and
+   unintuitive behaviours that make it hard to use correctly.
+
 ..
    vim: ft=rst tw=74 ai