[merge] Registry

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> On 12/10/2006, at 23:41 , Aaron Bentley wrote:
> 
>> Martin Pool wrote:
>>
>>> +1, except for a couple of grammatical fixes.  You have a habit of
>>> separating your clauses into sentences, as in this quoted section.  I
>>> don't mind how you write your email, but we should try to stick to
>>> standard English in documentation, and I'm pretty sure this isn't
>>> standard.
>>
>> Can we also get hardcore about apostrophes?  There's a certain core dev
>> who often omits them, and I always find that contractions which are
>> missing their apostrophes are quite jarring.
> 
> Let's get hardcode about good style for documentation/comments in general.
> 

Well, hopefully we'll get more 'hard-core' rather than 'hard-coded'. :)


>> "dont" and "cant" are
>> acceptable in symbol names, but I don't think they belong in doctexts or
>> comments.  (Though "cant" is a real word that means something else.)
> 
> Actually I find them a little distasteful in identifiers, and would
> either remove the negative (thing.can_eat_peanuts), or expand it
> (CannotDance).  The second is a bit harder to justify though.
> 
>> Getting more serious about grammar will make it a bit harder to merge
>> patches.  We might want to consider making integrators take
>> responsibility for tweaking grammar, because I don't want this to become
>> an extra hoop for non-native English speakers to jump through.
> 
> Well, one thing we can do is to give review comments but say "but I'll
> fix all of these when I merge them" -- this can apply to either code or
> doc changes.  Then other people know what was done and the original
> author gets the feedback, but we don't need to block.
> 
> If I'm reviewing English style or spelling I do try to give an
> alternative text.  Sometimes this just shows that I misunderstood the
> original and it can be more clear.
> 
>>>> +    This is used by Registry to make plain objects function similarly
>>>> +    to lazily imported objects.
>>
>> I'd be inclined to use hyphens for compound words like
>> "lazily-imported", which makes the binding crystal-clear.
> 
> The other day I saw an ad for a dress which said it was "ideal for lunch
> with the girls or after five drinks."
> 
>>>> +class _LazyObjectGetter(_ObjectGetter):
>>>> +    """Keep a record of a future object, and on request load it."""
>>
>> Should be "and, on request, load it."
> 
> I don't think it's incorrect as originally written, though yours might
> be clearer.  Sometimes I try to say "may be clearer as".

This seems to have too many commas:

Keep a record of a future object, and, on request, load it.

How about:

"""Keep a record of a possible object.

When requested, load and return it.
"""

> 
> More generally imperatives like this are appropriate for
> functions/methods, but less so for classes.  So it might be better as
> "Keeps a record".
> 
>>>> +    def get_obj(self):
>>>> +        """Get the referenced object.
>>>> +
>>>> +        If not imported yet, this will import the requested module.
>>
>> This has the object of the sentence hidden at the end of it.  I'd
>> prefer: "This will import the requested module, if not yet imported"
> 
> +1

Upon first request, the object will be imported. Future requests will
return the imported object.

>>>> +        :param help: Help text for this entry. This may be a string or
>>>> +                a callable. If it is a callable, it should take two
>>>> +                parameters, this registry and the key that the help
>>>> was
>>>> +                registered under.
>>
>> This should have a colon rather than a comma after 'parameters',
>> because the list 'this registry and the key...' describes 'parameters'.
> 
> +1

done

>>>> +        :param info: More information for this entry.
>>>> Registry.get_info()
>>>> +                can be used to get this information. It is meant as an
>>>> +                opaque storage location.
>>>> +        :param override_existing: If True, replace the existing object
>>
>> Should this be "If True, replace any existing..." ?
>>
>> Also, "is meant as" might be better as "should be treated as".
>>

The new text is:

:param key: This is the key to use to request the object later.
:param obj: The object to register.
:param help: Help text for this entry. This may be a string or
        a callable. If it is a callable, it should take two
        parameters (registry, key): this registry and the key that
        the help was registered under.
:param info: More information for this entry. Registry.get_info()
        can be used to get this information. Registry treats this as an
        opaque storage location (it is defined by the caller).
:param override_existing: Raise KeyErorr if False and something has
        already been registered for that key. If True, ignore if there
        is an existing key (always register the new value).


>>>> +    def register_lazy(self, key, module_name, member_name,
>>>> +                      help=None, info=None,
>>>> +                      override_existing=False):
>>>> +        """Register a new object to be loaded on request.
>>>> +
>>>> +        :param module_name: The python path to the module. Such as
>>>> 'os.path'.
>>>> +        :param member_name: The member of the module to return, if
>>>> empty or
>>>> +                None get() will return the module itself.
>>
>> This is a comma splice.  A semicolon, if not a period, should be here,
>> because there are two thoughts.  I'd write:
>>
>> :param member_name: The member of the module to return.  If empty or
>>         None, get() will return the module itself.
> 
> Right.  It's not so much about where there are commas as untangling the
> conditionals.
> 
> 
> --Martin

Done.

I'm happy to clean up documentation. Though I'm a little concerned it
may become one more hoop to jump through before we can merge anything.
And it is also a place that can be subject to a lot of bike-shedding.

I think the specific comments here are all very good. And if it stays at
this level, I think it will be an asset.

John
=:->



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFFLuUcJdeBCYSNAAMRArE1AKCuz0P2N+x5Sej4aBwjPHTcrSKC0gCgr8zy
ON93inFADqX18P5B34crxv8=
=0RR3
-----END PGP SIGNATURE-----