Rev 5780: Address review comments by jelmer and poolie. in file:///home/vila/src/bzr/experimental/config/
Vincent Ladeuil
v.ladeuil+lp at free.fr
Tue May 3 08:42:30 UTC 2011
At file:///home/vila/src/bzr/experimental/config/
------------------------------------------------------------
revno: 5780
revision-id: v.ladeuil+lp at free.fr-20110503084230-5y1o5f116rtsp6a0
parent: v.ladeuil+lp at free.fr-20110503075849-bw16zbtcz3voc3jc
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: config-abstract-store
timestamp: Tue 2011-05-03 10:42:30 +0200
message:
Address review comments by jelmer and poolie.
-------------- next part --------------
=== modified file 'bzrlib/config.py'
--- a/bzrlib/config.py 2011-05-03 07:58:49 +0000
+++ b/bzrlib/config.py 2011-05-03 08:42:30 +0000
@@ -2112,8 +2112,7 @@
def __repr__(self):
# Mostly for debugging use
- return "<%s.%s id=%s>" % (self.__module__, self.__class__.__name__,
- self.id)
+ return "<config.%s id=%s>" % (self.__class__.__name__, self.id)
_NewlyCreatedOption = object()
@@ -2144,14 +2143,19 @@
class Store(object):
"""Abstract interface to persistent storage for configuration options."""
- readonly_section_class = ReadOnlySection
+ readonly_section_class = Section
mutable_section_class = MutableSection
- @property
- def loaded(self):
- raise NotImplementedError(self.loaded)
+ def is_loaded(self):
+ """Returns True if the Store has been loaded.
+
+ This is used to implement lazy loading and ensure the persistent
+ storage is queried only when needed.
+ """
+ raise NotImplementedError(self.is_loaded)
def load(self):
+ """Loads the Store from persistent storage."""
raise NotImplementedError(self.load)
def _load_from_string(self, str_or_unicode):
@@ -2166,6 +2170,7 @@
raise NotImplementedError(self._load_from_string)
def save(self):
+ """Saves the Store to persistent storage."""
raise NotImplementedError(self.save)
def get_sections(self):
@@ -2182,8 +2187,21 @@
"""
raise NotImplementedError(self.get_mutable_section)
-
-class ConfigObjStore(Store):
+ def __repr__(self):
+ # Mostly for debugging use
+ return "<config.%s id=%s>" % (self.__class__.__name__, self.id)
+
+
+class IniFileStore(Store):
+ """A config Store using ConfigObj for storage.
+
+ :ivar transport: The transport object where the config file is located.
+
+ :ivar file_name: The config file basename in the transport directory.
+
+ :ivar _config_obj: Private member to hold the ConfigObj instance used to
+ serialize/deserialize the config file.
+ """
def __init__(self, transport, file_name):
"""A config Store using ConfigObj for storage.
@@ -2192,18 +2210,17 @@
:param file_name: The config file basename in the transport directory.
"""
- super(ConfigObjStore, self).__init__()
+ super(IniFileStore, self).__init__()
self.transport = transport
self.file_name = file_name
self._config_obj = None
- @property
- def loaded(self):
+ def is_loaded(self):
return self._config_obj != None
def load(self):
"""Load the store from the associated file."""
- if self.loaded:
+ if self.is_loaded():
return
content = self.transport.get_bytes(self.file_name)
self._load_from_string(content)
@@ -2217,7 +2234,7 @@
This is for tests and should not be used in production unless a
convincing use case can be demonstrated :)
"""
- if self.loaded:
+ if self.is_loaded():
raise AssertionError('Already loaded: %r' % (self._config_obj,))
co_input = StringIO(str_or_unicode.encode('utf-8'))
try:
@@ -2230,12 +2247,12 @@
# The following will do in the interim but maybe we don't want to
# expose a path here but rather a config ID and its associated
# object </hand wawe>.
- file_path = os.path.join(self.transport.external_url(),
- self.file_name)
+ file_path = urlutils.join(self.transport.external_url(),
+ self.file_name)
raise errors.ParseConfigError(e.errors, file_path)
def save(self):
- if not self.loaded:
+ if not self.is_loaded():
# Nothing to save
return
out = StringIO()
@@ -2275,7 +2292,7 @@
# they may face the same issue.
-class LockableConfigObjStore(ConfigObjStore):
+class LockableIniFileStore(IniFileStore):
"""A ConfigObjStore using locks on save to ensure store integrity."""
def __init__(self, transport, file_name, lock_dir_name=None):
@@ -2288,7 +2305,7 @@
if lock_dir_name is None:
lock_dir_name = 'lock'
self.lock_dir_name = lock_dir_name
- super(LockableConfigObjStore, self).__init__(transport, file_name)
+ super(LockableIniFileStore, self).__init__(transport, file_name)
self._lock = lockdir.LockDir(self.transport, self.lock_dir_name)
def lock_write(self, token=None):
@@ -2310,7 +2327,7 @@
@needs_write_lock
def save(self):
- super(LockableConfigObjStore, self).save()
+ super(LockableIniFileStore, self).save()
class cmd_config(commands.Command):
=== modified file 'bzrlib/tests/test_config.py'
--- a/bzrlib/tests/test_config.py 2011-05-03 07:58:49 +0000
+++ b/bzrlib/tests/test_config.py 2011-05-03 08:42:30 +0000
@@ -1886,8 +1886,8 @@
self.assertEquals(config._NewlyCreatedOption, section.orig['foo'])
-def get_ConfigObjStore(transport, file_name, content=None):
- """Build a ConfigObjStore.
+def get_IniFileStore(transport, file_name, content=None):
+ """Build a IniFileStore.
:param transport: The transport where the store lives.
@@ -1902,7 +1902,7 @@
implementations can rely on ConfigObj to parse the content and get the
option definitions and values from it.
"""
- store = config.ConfigObjStore(transport, file_name)
+ store = config.IniFileStore(transport, file_name)
if content is not None:
store._load_from_string(content)
return store
@@ -1921,7 +1921,7 @@
class TestReadonlyStore(TestStore):
- scenarios = [('configobj', {'_get_store': get_ConfigObjStore})]
+ scenarios = [('configobj', {'_get_store': get_IniFileStore})]
def get_store(self, file_name, content=None):
return self._get_store(
@@ -1930,9 +1930,9 @@
def test_delayed_load(self):
self.build_tree_contents([('foo.conf', '')])
store = self.get_store('foo.conf')
- self.assertEquals(False, store.loaded)
+ self.assertEquals(False, store.is_loaded())
store.load()
- self.assertEquals(True, store.loaded)
+ self.assertEquals(True, store.is_loaded())
def test_get_no_sections_for_empty(self):
store = self.get_store('foo.conf', '')
@@ -1958,7 +1958,7 @@
class TestMutableStore(TestStore):
- scenarios = [('configobj', {'_get_store': get_ConfigObjStore})]
+ scenarios = [('configobj', {'_get_store': get_IniFileStore})]
def get_store(self, file_name, content=None):
return self._get_store(
@@ -2021,28 +2021,28 @@
self.assertSectionContent(('baz', {'foo': 'bar'}), sections[0])
-class TestConfigObjStore(TestStore):
+class TestIniFileStore(TestStore):
def test_loading_unknown_file_fails(self):
- store = config.ConfigObjStore(self.get_transport(), 'I-do-not-exist')
+ store = config.IniFileStore(self.get_transport(), 'I-do-not-exist')
self.assertRaises(errors.NoSuchFile, store.load)
def test_invalid_content(self):
- store = config.ConfigObjStore(self.get_transport(), 'foo.conf', )
- self.assertEquals(False, store.loaded)
+ store = config.IniFileStore(self.get_transport(), 'foo.conf', )
+ self.assertEquals(False, store.is_loaded())
exc = self.assertRaises(
errors.ParseConfigError, store._load_from_string,
'this is invalid !')
self.assertEndsWith(exc.filename, 'foo.conf')
# And the load failed
- self.assertEquals(False, store.loaded)
+ self.assertEquals(False, store.is_loaded())
def test_get_embedded_sections(self):
# A more complicated example (which also shows that section names and
# option names share the same name space...)
# FIXME: This should be fixed by forbidding dicts as values ?
# -- vila 2011-04-05
- store = config.ConfigObjStore(self.get_transport(), 'foo.conf', )
+ store = config.IniFileStore(self.get_transport(), 'foo.conf', )
store._load_from_string('''
foo=bar
l=1,2
@@ -2071,17 +2071,17 @@
sections[3])
-class TestLockableConfigObjStore(TestStore):
+class TestLockableIniFileStore(TestStore):
def test_create_store_in_created_dir(self):
t = self.get_transport('dir/subdir')
- store = config.LockableConfigObjStore(t, 'foo.conf')
+ store = config.LockableIniFileStore(t, 'foo.conf')
store.get_mutable_section(None).set('foo', 'bar')
store.save()
# FIXME: We should adapt the tests in TestLockableConfig about concurrent
# writes. Since this requires a clearer rewrite, I'll just rely on using
- # the same code in LockableConfigObjStore (copied from LockableConfig, but
+ # the same code in LockableIniFileStore (copied from LockableConfig, but
# trivial enough, the main difference is that we add @needs_write_lock on
# save() instead of set_user_option() and remove_user_option()). The intent
# is to ensure that we always get a valid content for the store even when
=== modified file 'doc/developers/configuration.txt'
--- a/doc/developers/configuration.txt 2011-05-03 07:58:49 +0000
+++ b/doc/developers/configuration.txt 2011-05-03 08:42:30 +0000
@@ -24,15 +24,15 @@
Stores
------
-Options can persistent in which case they are saved into Stores.
+Options can be persistent in which case they are saved into Stores.
``config.Store`` defines the abstract interface that all stores should
implement.
-This object doesn't provide a direct access to the options, it only provides
-access to Sections. This is deliberate to ensure that sections can be properly
-shared by reusing the same underlying objects. Accessing options should be
-done via the ``Section`` objects.
+This object doesn't provide direct access to the options, it only provides
+access to Sections. This is deliberate to ensure that sections can be
+properly shared by reusing the same underlying objects. Accessing options
+should be done via the ``Section`` objects.
A ``Store`` can contain one or more sections, each section is uniquely
identified by a unicode string.
More information about the bazaar-commits
mailing list