[merge] LazyFactory
John Arbash Meinel
john at arbash-meinel.com
Mon Aug 14 17:34:32 BST 2006
Adeodato Simó wrote:
> * John Arbash Meinel [Wed, 09 Aug 2006 16:57:40 -0500]:
>
>> I think we have quite a few use cases for this sort of ability, and it
>> is best if we create a real factory class, and build up our lazy work
>> around it, rather than re-inventing the wheel each time.
>
> As I said on my message in the [RFC] Optparse thread [1], I think it
> would be good to generalize this LazyFactory idea into a registry-like
> class, and use subclasses for specialized stuff, like lazy imports.
>
> [1] https://lists.ubuntu.com/archives/bazaar-ng/2006q3/015765.html
>
> The attached patch (with tests) against John's initial version does
> that. Also attached for convenience is a diff for registry.py against
> bzr.dev.
>
> A couple comments:
>
> - John, I took the liberty to rename "Factory" into "Registry". The
> rationale is that it may not be a factory in some cases: eg., for my
> commit_template branch I'll be registering objects, not eg. classes.
>
> If you feel strong that is named "Factory", I guess it can be changed. ;-)
>
> - The get() method is a bit overloaded, but I wanted to support all
> possible use cases. Suggestions to improve it welcome.
>
> - [Minor? implementation detail] I moved from storing the default
> object in the dict under key None to store the default *key* (not
> object) in a separate property(). I really think that the default
> should be a key, not an object.
>
> I'd be seeking +1 on this, unless John doesn't like what I did with his
> code. If this goes into bzr.dev, I'll look into a OptionValuesRegistry
> and OptionFromRegistry, and maybe other useful ones (eg. one that raises
> BzrCommandError if the key is not found).
>
> Mergeable URL:
>
> http://people.debian.org/~adeodato/code/branches/bzr/registry_class/
>
> Cheers,
...
> + 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
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]
> +
> + def keys(self):
> + """Get a list of registered entries"""
> + return sorted(self._dict.keys())
> +
> + def _set_default_key(self, key):
> + if not self._dict.has_key(key):
> + raise KeyError('No object registered under key %s.' % key)
> + else:
> + self._default_key = key
> +
> + def _get_default_key(self):
> + return self._default_key
> +
> + default_key = property(_get_default_key, _set_default_key)
> + """Current value of the default key. Can be set to any existing key."""
> +
> +
> +class LazyImportRegistry(Registry):
> + """A class to register modules/members to be loaded on request."""
> +
> def register(self, key, module_name, member_name):
> """Register a new object to be loaded on request.
>
> - :param key: This is the key to use to request the object later.
> - :param module_name: The python path to the module. Such as 'os.path'
> + :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.
> - :return: if something used to be registered, its information will be
> - returned.
> """
> - old_info = self._dict.get(key)
> - if self._first_is_default and not self._dict:
> - self._dict[None] = (module_name, member_name)
> - self._dict[key] = (module_name, member_name)
> + Registry.register(self, key, (module_name, member_name))
>
> - def get(self, key=None):
> + def get(self, key=None, default_key=_marker):
> """Load the module and return the object specified by the given key.
>
> - This may raise KeyError if the key is not present.
> - May also raise ImportError if there are any problems
> - Or AttributeError if the module does not have the supplied member
> -
> - :param key: The key registered by register()
> - :return: The module/klass/function/object specified earlier
> + May raise ImportError if there are any problems, or AttributeError if
> + the module does not have the supplied member.
> """
> - module_name, member_name = self._dict[key]
> + module_name, member_name = Registry.get(self, key, default_key)
> module = __import__(module_name, globals(), locals(), [member_name])
> if member_name:
> return getattr(module, member_name)
> return module
> -
> - def keys(self, include_none=False):
> - """Get a list of registered entries"""
> - keys = self._dict.keys()
> - if not include_none and None in self._dict:
> - keys.remove(None)
> - return keys
>
> === renamed file 'bzrlib/tests/test_lazy_factory.py' => 'bzrlib/tests/test_registry.py'
> --- bzrlib/tests/test_lazy_factory.py 2006-08-09 21:37:43 +0000
> +++ bzrlib/tests/test_registry.py 2006-08-10 21:47:27 +0000
> @@ -14,20 +14,68 @@
> # along with this program; if not, write to the Free Software
> # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>
> -"""Tests for the LazyFactory class"""
> +"""Tests for the Registry classes"""
>
> import os
> import sys
>
> from bzrlib import (
> errors,
> - lazy_factory,
> + registry,
> osutils,
> )
> -from bzrlib.tests import TestCaseInTempDir
> -
> -
> -class TestLazyFactory(TestCaseInTempDir):
> +from bzrlib.tests import TestCase, TestCaseInTempDir
> +
> +
> +class TestRegistry(TestCase):
> + def register_stuff(self, registry):
> + registry.register('one', 1)
> + registry.register('two', 2)
> + registry.register('four', 4)
> + registry.register('five', 5)
> +
> + def test_registry(self):
> + registry_ = registry.Registry()
> + self.register_stuff(registry_)
> +
> + self.failUnless(registry_.default_key is None)
> +
> + # test get() (self.default_key == None)
> + self.assertRaises(KeyError, registry_.get)
> + self.assertEqual(2, registry_.get('two'))
> + self.assertRaises(KeyError, registry_.get, 'three')
> + self.assertEqual(4, registry_.get('three', 'four'))
> + self.assertRaises(KeyError, registry_.get, 'three', 'no-such-key')
> +
> + # test _set_default_key
> + registry_.default_key = 'five'
> + self.failUnless(registry_.default_key == 'five')
> + self.assertEqual(5, registry_.get('six'))
> + self.assertRaises(KeyError, registry_._set_default_key, 'six')
> +
> + # test keys()
> + self.assertEqual(['five', 'four', 'one', 'two'], registry_.keys())
> +
> + def test_registry_with_first_is_default(self):
> + registry_ = registry.Registry(True)
> + self.register_stuff(registry_)
> +
> + self.failUnless(registry_.default_key == 'one')
> +
> + # test get() (self.default_key == 'one')
> + self.assertEqual(1, registry_.get())
> + self.assertEqual(2, registry_.get('two'))
> + self.assertEqual(1, registry_.get('three'))
> + self.assertEqual(4, registry_.get('three', 'four'))
> + self.assertRaises(KeyError, registry_.get, 'three', 'no-such-key')
> +
> + # test _set_default_key
> + registry_.default_key = 'five'
> + self.failUnless(registry_.default_key == 'five')
> + self.assertEqual(5, registry_.get('six'))
> + self.assertRaises(KeyError, registry_._set_default_key, 'six')
> +
> +class TestLazyImportRegistry(TestCaseInTempDir):
>
> def create_plugin_file(self, contents):
> plugin_name = 'bzr_plugin_a_%s' % (osutils.rand_chars(4),)
> @@ -47,9 +95,9 @@
> '\n\n'
> )
>
> - def test_lazy_factory(self):
> + def test_lazy_import_registry(self):
> plugin_name = self.create_simple_plugin()
> - factory = lazy_factory.LazyFactory()
> + factory = registry.LazyImportRegistry()
> factory.register('obj', plugin_name, 'object1')
> factory.register('function', plugin_name, 'function')
> factory.register('klass', plugin_name, 'MyClass')
>
> === modified file 'bzrlib/tests/__init__.py'
> --- bzrlib/tests/__init__.py 2006-08-10 19:46:14 +0000
> +++ bzrlib/tests/__init__.py 2006-08-10 21:47:27 +0000
> @@ -1287,7 +1287,7 @@
> 'bzrlib.tests.test_ignores',
> 'bzrlib.tests.test_inv',
> 'bzrlib.tests.test_knit',
> - 'bzrlib.tests.test_lazy_factory',
> + 'bzrlib.tests.test_registry',
> 'bzrlib.tests.test_lockdir',
> 'bzrlib.tests.test_lockable_files',
> 'bzrlib.tests.test_log',
>
>
>
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.
A few more comments...
What does Registry provide over a plain dictionary?
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)
Though I suppose you played with that by allowing:
obj = registry.get(key, None)
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).
John
=:->
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060814/54b86b6a/attachment.pgp
More information about the bazaar
mailing list