[merge] LazyFactory

Adeodato Simó dato at net.com.org.es
Tue Aug 15 03:14:47 BST 2006


* John Arbash Meinel [Mon, 14 Aug 2006 11:34:32 -0500]:

> > +    def register(self, key, object):
> > +        """Register a new object to a name.
> > +
> > +        :param key: This is the key to use to request the object later.
> > +        :param object: The object to register.
> > +        """
> > +        if self._first_is_default and not self._dict:
> > +            self._default_key = key
> > +        self._dict[key] = object

> If you are going to have an explicit _default_key object, then this
> should be:

> if self._first_is_default and self._default_key is None:
>   self._default_key = key

Well, while I see your point, I think bool(self._dict) reads closer to
"is this the first registered entry?", than checking _default_key. But
if you're strong on having it with "_default_key is None", I don't
object at all to changing it.

> Sending 2 bundles makes it a little confusing what should be reviewed. I
> realize you are just being thorough. Maybe send the rollup as a
> patch.gz, so that it doesn't show up, but can be used to merge from.

Okay, sorry for that.

> What does Registry provide over a plain dictionary?

A way to implement the "register_function/get_function/list_available"
pattern in a consistent way all over bzrlib (modules export just a
Registry object instead of their pair of functions). Also, with
standardization, stuff can be built on top of it, like FromRegistryOption.

> v- I don't really prefer being able to pass in what the default key is
> as part of 'get()'. If you really want that, then it should be done in
> another location. (Say at 'init' time). Looking over it, you have about
> 3 different ways to specify what the default key is. I think we would
> want only 1. I could see 'get()' having a 'default_value' parameter, in
> the case that a key didn't exist. But not a 'default_key' parameter.

> > +
> > +    def get(self, key=_marker, default_key=_marker):
> > +        """Return the object register()'ed by the given key.
> > +
> > +        This may raise KeyError if the key is not present.
> > +
> > +        :param key: The key to obtain the object for; if not given, :param
> > +            default_key: will be used.
> > +        :param default_key: Key to use if an object for :param key: can't be
> > +            found; defaults to self.default_key. Set it to None if you'd like
> > +            to ensure an exception is raised for non-found keys.
> > +        :return: The previously registered object.
> > +        """
> > +        if default_key is _marker:
> > +            default_key = self.default_key
> > +
> > +        if key is _marker:
> > +            return self._dict[default_key]
> > +        else:
> > +            try:
> > +                return self._dict[key]
> > +            except KeyError:
> > +                return self._dict[default_key]
> > +

On perspective, the name of the parameter should probably be "fallback_key"
instead of "default_key". I've changed that in my branch.

The intent is to supply the functionality of default_value, but assuming
such value would always come from the registry itself, so that instead
of e.g.:

  foo = registry.get(user_supplied, registry.get(another_one))

you could do:

  foo = registry.get(user_supplied, another_one)

> I would rather the default always be key=None, because then clients have
> a chance to pass that in. So calling code is:

> key = <get some key from the user in some fashion>
> obj = registry.get(key)

> Otherwise, callers have to do:

> key = <get some key from the user in some fashion>
> if key is None:
>   obj = registry.get()
> else:
>   obj = registry.get(key)

Oh, good point. I've fixed that (diff included below; mergeable from [1]).

  [1] http://people.debian.org/~adeodato/code/branches/bzr/registry_class

> Anyway, I'm happy with the idea of creating a different base class, as
> long as it does something useful. I don't know that having lots of
> different ways to specify a default is useful. I suppose having the
> abstraction lets you swap out one Registry type for another if you want
> different functionality later on. (Like failing if a key already exists,
> or returning the original value from the register() function).

Nice. So to clear things up, this should be ready to enter after you
tell me your preference about this two issues:

  - the register() bit mentioned at the beginning of this mail

  - whether to drop fallback_key in get(), and whether to replace it
    with default_value=None (in which way, as a side-effect, the method
    would never raise a KeyError exception, but return None instead).

I hope this can make it into 0.10, maybe even with making log.py use it.
BTW, would this addition need a NEWS entry under "INTERNALS"?

Thanks,

-- 
Adeodato Simó                                     dato at net.com.org.es
Debian Developer                                  adeodato at debian.org
 
      Listening to: Los Planetas - Pesadilla en el parque de atracciones





More information about the bazaar mailing list