Bazaar

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

hooks-refactoring
Merge into bzr.dev

Revision history for this message

Andrew Bennetts (spiv) wrote on 2010-09-28:

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 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
        'os.path'.

> Nice removal of duplications there.

Thanks!

-Andrew.

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.

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 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
        'os.path'.

> Nice removal of duplications there.

Thanks!

-Andrew.

« Back to merge proposal