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

Revision history for this message
Vincent Ladeuil (vila) wrote :

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,

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

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.

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 ;)

review: Needs Fixing

« Back to merge proposal