Code review comment for lp:~jelmer/bzr/lazy-registries

Am 29/03/12 10:27, schrieb Vincent Ladeuil:
> Review: Needs Fixing
>
> Nice one.
>
> I think I'm not comfortable with two specifics points:
>
> - you've added a feature to Registry, I think it would be clearer to create
> a new class instead,
If we add a new class, then that means that any users of registries
would either have to stick with the old implementation *or* be converted
to the new one. Adding it to the current class just allows us to update
the sites and then plugins can use either the lazy registration or
non-lazy registration as they see fit. We wouldn't have to immediately
update all plugins, their existing calls would still just work.
>
> - the way it works is magical for people that don't know how lazy loading
> works, a few comments or docstrings should address that.
>
> 565 +RegistryEntry = collections.namedtuple('RegistryEntry', 'objgetter info help')
>
> Good addition but Worth a docstring.
Fixed.
>
> 583 + if module is not None and member_name is not None:
>
> This is the line that conflates two classes into a single one for a benefit
> that is not clear to me.
If module is not set, then the Registry() object only allows for direct
registration (i.e. importing the object itself and calling
install_named_hook or install_lazy_named_hook on it). If
module/member_name is set, then it also supports install_lazy_hook() *in
addition* to the other methods.

I've added a bit of documentation to the Registry docstring.

>
> 688 +# Lazily registered registries. Maps (module, registry_name, name,
> 689 +# entry_module, entry_name) -> (objgetter, info, help)
> 690 +_lazy_registries = {}
>
> This is where you should explain part of the magic.
>
> 712 + obj_getter = _LazyObjectGetter(entry_module, entry_name)
> 713 + registry_dict[key] = RegistryEntry(obj_getter, info, help)
>
> And this is the othe place where the magic takes place (truth is there is
> yet another place where the magic occurs but it won't appear that magical if
> you use a different class). Someone not knowing about lazy loading and
> obj_getter is likely to be completely lost ;)
I've updated the comments.

Cheers,

Jelmer