Rev 5450: Simplify code and design by only defining get_options() where relevant. in file:///home/vila/src/bzr/experimental/config/
Vincent Ladeuil
v.ladeuil+lp at free.fr
Fri Oct 1 10:52:08 BST 2010
At file:///home/vila/src/bzr/experimental/config/
------------------------------------------------------------
revno: 5450
revision-id: v.ladeuil+lp at free.fr-20101001095208-vupdrq1jtle74gk7
parent: v.ladeuil+lp at free.fr-20100930171515-k6ablrxpu2ei3ipz
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: config-read
timestamp: Fri 2010-10-01 11:52:08 +0200
message:
Simplify code and design by only defining get_options() where relevant.
-------------- next part --------------
=== modified file 'NEWS'
--- a/NEWS 2010-09-30 17:15:15 +0000
+++ b/NEWS 2010-10-01 09:52:08 +0000
@@ -43,10 +43,9 @@
API Changes
***********
-* ``bzrlib.config.IniBasedConfig`` objects now implement
- ``get_options_matching_glob`` and ``get_options_matching_regexp`` that
- permit querying them for matching variables, their values and which
- configuration file define them. (Vincent Ladeuil)
+* ``bzrlib.config.IniBasedConfig`` objects now implement ``get_options`` that
+ permit querying them for the options defined and their values.
+ (Vincent Ladeuil)
Internals
*********
=== modified file 'bzrlib/config.py'
--- a/bzrlib/config.py 2010-09-30 17:15:15 +0000
+++ b/bzrlib/config.py 2010-10-01 09:52:08 +0000
@@ -155,6 +155,10 @@
def __init__(self):
super(Config, self).__init__()
+ def id(self):
+ """Returns a unique ID for the config."""
+ raise NotImplementedError(self.id)
+
def get_editor(self):
"""Get the users pop up editor."""
raise NotImplementedError
@@ -446,37 +450,19 @@
"""Override this to define the section used by the config."""
return "DEFAULT"
- def get_options_matching_glob(self, name_glob, sections=None,
- file_name=None):
- """Return an ordered list of (name, value, section, file_name) tuples.
-
- This is a convenience on top of ``get_options_matching_regexp``
- accepting a glob.
- """
- name_re = re.compile(fnmatch.translate(name_glob))
- return self.get_options_matching_regexp(name_re, sections=sections,
- file_name=file_name)
-
- def get_options_matching_regexp(self, name_re, sections=None,
- file_name=None):
- """Return an ordered list of (name, value, section, file_name) tuples.
-
- All variables whose name match ``name_re`` are returned with their
- associated value and the section they appeared in.
+ def get_options(self, sections=None):
+ """Return an ordered list of (name, value, section, config_id) tuples.
+
+ All options are returned with their associated value and the section
+ they appeared in. ``config_id`` is a unique identifier for the
+ configuration file the option is defined in.
:param sections: Default to ``_get_matching_sections`` if not
specified. This gives a better control to daughter classes about
- which sections should be searched.
-
- :param file_name: Which file path should be used in the returned list.
+ which sections should be searched. This is a list of (name,
+ configobj) tuples.
"""
- # FIXME: file_name isn't very user friendly, the basename should
- # probably be displayed (by higher levels) but that won't fly for
- # branch.conf if several branches are involved (bound branches or heady
- # checkouts will want to distinguish the master branch). Using the
- # absolute path should be good enough for a first implementation.
- # -- vila 20100930
- matches = []
+ opts = []
if sections is None:
parser = self._get_parser()
sections = []
@@ -489,13 +475,10 @@
# instead ? -- vila 20100930
continue
sections.append((section_name, section))
- if file_name is None:
- file_name = self.file_name
+ config_id = self.id()
for (section_name, section) in sections:
for (name, value) in section.iteritems():
- if name_re.search(name):
- matches.append((name, value, section_name, file_name))
- return matches
+ yield (name, value, section_name, config_id)
def _get_option_policy(self, section, option_name):
"""Return the policy for the given (section, option_name) pair."""
@@ -671,6 +654,9 @@
def __init__(self):
super(GlobalConfig, self).__init__(file_name=config_filename())
+ def id(self):
+ return 'bazaar'
+
@classmethod
def from_string(cls, str_or_unicode, save=False):
"""Create a config object from a string.
@@ -733,6 +719,9 @@
location = urlutils.local_path_from_url(location)
self.location = location
+ def id(self):
+ return 'locations'
+
@classmethod
def from_string(cls, str_or_unicode, location, save=False):
"""Create a config object from a string.
@@ -878,6 +867,9 @@
self._get_branch_data_config,
self._get_global_config)
+ def id(self):
+ return 'branch'
+
def _get_branch_data_config(self):
if self._branch_data_config is None:
self._branch_data_config = TreeConfig(self.branch)
@@ -954,43 +946,24 @@
return value
return None
- def get_options_matching_glob(self, name_glob, sections=None,
- file_name=None):
- name_re = re.compile(fnmatch.translate(name_glob))
- return self.get_options_matching_regexp(name_re, sections=sections,
- file_name=file_name)
-
- def get_options_matching_regexp(self, name_re, sections=None,
- file_name=None):
- matches = []
- location_config = self._get_location_config()
- matches.extend(location_config.get_options_matching_regexp(name_re))
- # FIXME: The following is rather convoluted but
- # BranchConfig/TreeConfig/TransportConfig separation doesn't provide an
- # easier access. Now that GlobalConfig and Location_Config are
- # LockableConfig objects that already requires a transport, we may want
- # to refactor BranchConfig to get a simpler implementation
- # -- vila 20100930
+ def get_options(self, sections=None):
+ opts = []
+ # First the locations options
+ for option in self._get_location_config().get_options():
+ yield option
+ # Then the branch options
branch_config = self._get_branch_data_config()
if sections is None:
sections = [('DEFAULT', branch_config._get_parser())]
- if file_name is None:
- try:
- file_name = self.file_name
- except AttributeError:
- # FIXME: Let's stop the horror right now, neither BranchConfig
- # nor RemoteBranchConfig provides an easy way to get the file
- # name which would be an url or an absolute path anyway. Let's
- # punt for now until we decide how we want to report this piece
- # of into to the user (preferably in a short form so 'bzr
- # config var=value --in bazaar|location|branch remains easy to
- # use) --vila 20100930
- file_name = 'branch.conf'
- matches.extend(branch_config.get_options_matching_regexp(
- name_re, sections, file_name))
- global_config = self._get_global_config()
- matches.extend(global_config.get_options_matching_regexp(name_re))
- return matches
+ # FIXME: We shouldn't have to duplicate the code in IniBasedConfig but
+ # Config itself has no notion of sections :( -- vila 20101001
+ config_id = self.id()
+ for (section_name, section) in sections:
+ for (name, value) in section.iteritems():
+ yield (name, value, section_name, config_id)
+ # Then the global options
+ for option in self._get_global_config().get_options():
+ yield option
def set_user_option(self, name, value, store=STORE_BRANCH,
warn_masked=False):
@@ -1697,16 +1670,16 @@
"""
aliases = ['conf']
- takes_args = ['options?']
+ takes_args = ['matching?']
takes_options = [
'directory',
]
@commands.display_command
- def run(self, options=None, directory=None):
- if options is None:
- options = '*'
+ def run(self, matching=None, directory=None):
+ if matching is None:
+ matching = '*'
if directory is None:
directory = '.'
try:
@@ -1714,12 +1687,14 @@
confs = [br.get_config()]
except errors.NotBranchError:
confs = [LocationConfig(directory), GlobalConfig()]
+ # Turn the glob into a regexp
+ matching_re = re.compile(fnmatch.translate(matching))
+ cur_conf_id = None
for c in confs:
- matches = c.get_options_matching_glob(options)
- cur_file_name = None
- for (option_name, value, section, file_name) in matches:
- if cur_file_name != file_name:
- self.outf.write('%s:\n' % (file_name,))
- cur_file_name = file_name
- self.outf.write(' %s = %s\n' % (option_name, value))
+ for (name, value, section, conf_id) in c.get_options():
+ if matching_re.search(name):
+ if cur_conf_id != conf_id:
+ self.outf.write('%s:\n' % (conf_id,))
+ cur_conf_id = conf_id
+ self.outf.write(' %s = %s\n' % (name, value))
=== modified file 'bzrlib/tests/blackbox/test_config.py'
--- a/bzrlib/tests/blackbox/test_config.py 2010-09-30 17:15:15 +0000
+++ b/bzrlib/tests/blackbox/test_config.py 2010-10-01 09:52:08 +0000
@@ -55,7 +55,7 @@
self.global_config.set_user_option('hello', 'world')
script.run_script(self, '''
$ bzr config -d tree
-...bazaar.conf:
+bazaar:
hello = world
''')
@@ -64,9 +64,9 @@
self.branch_config.set_user_option('hello', 'you')
script.run_script(self, '''
$ bzr config -d tree
-...locations.conf:
+locations:
hello = world
-...branch.conf:
+branch:
hello = you
''')
@@ -75,6 +75,6 @@
self.locations_config.set_user_option('hello', 'world')
script.run_script(self, '''
$ bzr config
-...bazaar.conf:
+bazaar:
hello = world
''')
=== modified file 'bzrlib/tests/test_config.py'
--- a/bzrlib/tests/test_config.py 2010-09-30 11:10:52 +0000
+++ b/bzrlib/tests/test_config.py 2010-10-01 09:52:08 +0000
@@ -1471,71 +1471,65 @@
self.assertIs(None, bzrdir_config.get_default_stack_on())
-class TestConfigDisplay(tests.TestCaseWithTransport):
+class TestConfigGetOptions(tests.TestCaseWithTransport):
def setUp(self):
- super(TestConfigDisplay, self).setUp()
+ super(TestConfigGetOptions, self).setUp()
self.global_config = config.GlobalConfig()
self.tree = self.make_branch_and_tree('.')
self.branch_config = config.BranchConfig(self.tree.branch)
self.locations_config = config.LocationConfig(self.tree.basedir)
- def assertMatchingVars(self, expected, conf, name_glob):
- actual = conf.get_options_matching_glob(name_glob)
- # Filter the config file name as we don't care here
- filtered = [(name, value, section)
- for name, value, section, file_name in actual]
- self.assertEqual(expected, filtered)
+ def assertOptions(self, expected, conf):
+ actual = list(conf.get_options())
+ self.assertEqual(expected, actual)
# One variable in non of the above
def test_no_variable(self):
# Using branch should query branch, locations and bazaar
- self.assertMatchingVars([], self.branch_config, 'file')
-
- def test_no_matches(self):
- self.global_config.set_user_option('file', 'bazaar')
- self.locations_config.set_user_option('file', 'locations')
- self.branch_config.set_user_option('file', 'branch')
- self.assertMatchingVars([], self.branch_config, 'not_file')
-
- def test_variable_in_bazaar(self):
- self.global_config.set_user_option('file', 'bazaar')
- self.assertMatchingVars([('file', 'bazaar', 'DEFAULT')],
- self.global_config, 'file')
-
- def test_variable_in_locations(self):
- self.locations_config.set_user_option('file', 'locations')
- self.assertMatchingVars([('file', 'locations', self.tree.basedir)],
- self.locations_config, 'file')
-
- def test_variable_in_branch(self):
- self.branch_config.set_user_option('file', 'branch')
- self.assertMatchingVars([('file', 'branch', 'DEFAULT')],
- self.branch_config, 'file')
-
- def test_variable_in_bazaar_and_branch(self):
- self.global_config.set_user_option('file', 'bazaar')
- self.branch_config.set_user_option('file', 'branch')
- self.assertMatchingVars([('file', 'branch', 'DEFAULT'),
- ('file', 'bazaar', 'DEFAULT'),],
- self.branch_config, 'file')
-
- def test_variable_in_branch_and_locations(self):
+ self.assertOptions([], self.branch_config)
+
+ def test_option_in_bazaar(self):
+ self.global_config.set_user_option('file', 'bazaar')
+ self.assertOptions([('file', 'bazaar', 'DEFAULT', 'bazaar')],
+ self.global_config)
+
+ def test_option_in_locations(self):
+ self.locations_config.set_user_option('file', 'locations')
+ self.assertOptions(
+ [('file', 'locations', self.tree.basedir, 'locations')],
+ self.locations_config)
+
+ def test_option_in_branch(self):
+ self.branch_config.set_user_option('file', 'branch')
+ self.assertOptions([('file', 'branch', 'DEFAULT', 'branch')],
+ self.branch_config)
+
+ def test_option_in_bazaar_and_branch(self):
+ self.global_config.set_user_option('file', 'bazaar')
+ self.branch_config.set_user_option('file', 'branch')
+ self.assertOptions([('file', 'branch', 'DEFAULT', 'branch'),
+ ('file', 'bazaar', 'DEFAULT', 'bazaar'),],
+ self.branch_config)
+
+ def test_option_in_branch_and_locations(self):
# Hmm, locations override branch :-/
self.locations_config.set_user_option('file', 'locations')
self.branch_config.set_user_option('file', 'branch')
- self.assertMatchingVars([('file', 'locations', self.tree.basedir),
- ('file', 'branch', 'DEFAULT'),],
- self.branch_config, 'file')
+ self.assertOptions(
+ [('file', 'locations', self.tree.basedir, 'locations'),
+ ('file', 'branch', 'DEFAULT', 'branch'),],
+ self.branch_config)
- def test_variable_in_bazaar_locations_and_branch(self):
+ def test_option_in_bazaar_locations_and_branch(self):
self.global_config.set_user_option('file', 'bazaar')
self.locations_config.set_user_option('file', 'locations')
self.branch_config.set_user_option('file', 'branch')
- self.assertMatchingVars([('file', 'locations', self.tree.basedir),
- ('file', 'branch', 'DEFAULT'),
- ('file', 'bazaar', 'DEFAULT'),],
- self.branch_config, 'file')
+ self.assertOptions(
+ [('file', 'locations', self.tree.basedir, 'locations'),
+ ('file', 'branch', 'DEFAULT', 'branch'),
+ ('file', 'bazaar', 'DEFAULT', 'bazaar'),],
+ self.branch_config)
class TestAuthenticationConfigFile(tests.TestCase):
More information about the bazaar-commits
mailing list