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

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

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

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

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

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

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

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.

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

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

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

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.