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

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

--
Martin