Rev 6087: Merge fix for default value conversion in file:///home/vila/src/bzr/experimental/expand-in-stack/

Vincent Ladeuil v.ladeuil+lp at free.fr
Wed Aug 24 18:49:36 UTC 2011


At file:///home/vila/src/bzr/experimental/expand-in-stack/

------------------------------------------------------------
revno: 6087 [merge]
revision-id: v.ladeuil+lp at free.fr-20110824184935-nzwwcd90r6j3s2ib
parent: v.ladeuil+lp at free.fr-20110824184215-h20vqa9qbksnb2jy
parent: v.ladeuil+lp at free.fr-20110822161530-0fjr0b9ddioyru26
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: expand-in-stack
timestamp: Wed 2011-08-24 20:49:35 +0200
message:
  Merge fix for default value conversion
modified:
  bzrlib/config.py               config.py-20051011043216-070c74f4e9e338e8
  bzrlib/tests/test_config.py    testconfig.py-20051011041908-742d0c15d8d8c8eb
-------------- next part --------------
=== modified file 'bzrlib/config.py'
--- a/bzrlib/config.py	2011-08-24 18:42:15 +0000
+++ b/bzrlib/config.py	2011-08-24 18:49:35 +0000
@@ -2322,7 +2322,7 @@
         :param name: the name used to refer to the option.
 
         :param default: the default value to use when none exist in the config
-            stores.
+            stores. This must be a string as it appears in the config stores.
 
         :param default_from_env: A list of environment variables which can
            provide a default value. 'default' will be used only if none of the
@@ -2352,13 +2352,37 @@
             raise AssertionError("%s not supported for 'invalid'" % (invalid,))
         self.invalid = invalid
 
+    def convert_from_unicode(self, unicode_value):
+        if self.from_unicode is None or unicode_value is None:
+            # Don't convert or nothing to convert
+            return unicode_value
+        try:
+            converted = self.from_unicode(unicode_value)
+        except (ValueError, TypeError):
+            # Invalid values are ignored
+            converted = None
+        if converted is None and self.invalid is not None:
+            # The conversion failed
+            if self.invalid == 'warning':
+                trace.warning('Value "%s" is not valid for "%s"',
+                              unicode_value, self.name)
+            elif self.invalid == 'error':
+                raise errors.ConfigOptionValueError(self.name, unicode_value)
+        return converted
+
     def get_default(self):
+        value = None
         for var in self.default_from_env:
             try:
-                return os.environ[var]
+                # If the env variable is defined, its value is the default one
+                value = os.environ[var]
+                break
             except KeyError:
                 continue
-        return self.default
+        if value is None:
+            # Otherwise, fallback to the value defined at registration
+            value = self.default
+        return value
 
     def get_help_text(self, additional_see_also=None, plain=True):
         result = self.help
@@ -2439,7 +2463,7 @@
 # Registered options in lexicographical order
 
 option_registry.register(
-    Option('bzr.workingtree.worth_saving_limit', default=10,
+    Option('bzr.workingtree.worth_saving_limit', default='10',
            from_unicode=int_from_store,  invalid='warning',
            help='''\
 How many changes before saving the dirstate.
@@ -2451,7 +2475,7 @@
 a file has been touched.
 '''))
 option_registry.register(
-    Option('dirstate.fdatasync', default=True,
+    Option('dirstate.fdatasync', default='True',
            from_unicode=bool_from_store,
            help='''\
 Flush dirstate changes onto physical disk?
@@ -2461,7 +2485,7 @@
 should not be lost if the machine crashes.  See also repository.fdatasync.
 '''))
 option_registry.register(
-    Option('debug_flags', default=[], from_unicode=list_from_store,
+    Option('debug_flags', default='', from_unicode=list_from_store,
            help='Debug flags to activate.'))
 option_registry.register(
     Option('default_format', default='2a',
@@ -2470,7 +2494,7 @@
     Option('editor',
            help='The command called to launch an editor to enter a message.'))
 option_registry.register(
-    Option('ignore_missing_extensions', default=False,
+    Option('ignore_missing_extensions', default='False',
            from_unicode=bool_from_store,
            help='''\
 Control the missing extensions warning display.
@@ -2481,7 +2505,7 @@
     Option('language',
            help='Language to translate messages into.'))
 option_registry.register(
-    Option('locks.steal_dead', default=False, from_unicode=bool_from_store,
+    Option('locks.steal_dead', default='False', from_unicode=bool_from_store,
            help='''\
 Steal locks that appears to be dead.
 
@@ -2497,7 +2521,7 @@
            help= 'Unicode encoding for output'
            ' (terminal encoding if not specified).'))
 option_registry.register(
-    Option('repository.fdatasync', default=True, from_unicode=bool_from_store,
+    Option('repository.fdatasync', default='True', from_unicode=bool_from_store,
            help='''\
 Flush repository changes onto physical disk?
 
@@ -2988,22 +3012,6 @@
         except KeyError:
             # Not registered
             opt = None
-        if (opt is not None and opt.from_unicode is not None
-            and value is not None):
-            # If a value exists and the option provides a converter, use it
-            try:
-                converted = opt.from_unicode(value)
-            except (ValueError, TypeError):
-                # Invalid values are ignored
-                converted = None
-            if converted is None and opt.invalid is not None:
-                # The conversion failed
-                if opt.invalid == 'warning':
-                    trace.warning('Value "%s" is not valid for "%s"',
-                                  value, name)
-                elif opt.invalid == 'error':
-                    raise errors.ConfigOptionValueError(name, value)
-            value = converted
         if value is None:
             # If the option is registered, it may provide a default value
             if opt is not None:
@@ -3017,6 +3025,12 @@
                               % (name,))
             elif isinstance(value, (str, unicode)):
                 value = self._expand_options_in_string(value)
+        if opt is not None:
+            value = opt.convert_from_unicode(value)
+            if value is None:
+                # The conversion failed or there was no value to convert,
+                # fallback to the default value
+                value = opt.convert_from_unicode(opt.get_default())
         for hook in ConfigHooks['get']:
             hook(self, name, value)
         return value

=== modified file 'bzrlib/tests/test_config.py'
--- a/bzrlib/tests/test_config.py	2011-08-24 18:42:15 +0000
+++ b/bzrlib/tests/test_config.py	2011-08-24 18:49:35 +0000
@@ -2259,7 +2259,7 @@
         self.overrideEnv('FOO', 'quux')
         # Env variable provides a default taking over the option one
         self.assertEquals('quux', opt.get_default())
-        
+
     def test_first_default_value_from_env_wins(self):
         opt = config.Option('foo', default='bar',
                             default_from_env=['NO_VALUE', 'FOO', 'BAZ'])
@@ -2269,6 +2269,99 @@
         self.assertEquals('foo', opt.get_default())
 
 
+class TestOptionConverterMixin(object):
+
+    def assertConverted(self, expected, opt, value):
+        self.assertEquals(expected, opt.convert_from_unicode(value))
+
+    def assertWarns(self, opt, value):
+        warnings = []
+        def warning(*args):
+            warnings.append(args[0] % args[1:])
+        self.overrideAttr(trace, 'warning', warning)
+        self.assertEquals(None, opt.convert_from_unicode(value))
+        self.assertLength(1, warnings)
+        self.assertEquals(
+            'Value "%s" is not valid for "%s"' % (value, opt.name),
+            warnings[0])
+
+    def assertErrors(self, opt, value):
+        self.assertRaises(errors.ConfigOptionValueError,
+                          opt.convert_from_unicode, value)
+
+    def assertConvertInvalid(self, opt, invalid_value):
+        opt.invalid = None
+        self.assertEquals(None, opt.convert_from_unicode(invalid_value))
+        opt.invalid = 'warning'
+        self.assertWarns(opt, invalid_value)
+        opt.invalid = 'error'
+        self.assertErrors(opt, invalid_value)
+
+
+class TestOptionWithBooleanConverter(tests.TestCase, TestOptionConverterMixin):
+
+    def get_option(self):
+        return config.Option('foo', help='A boolean.',
+                             from_unicode=config.bool_from_store)
+
+    def test_convert_invalid(self):
+        opt = self.get_option()
+        # A string that is not recognized as a boolean
+        self.assertConvertInvalid(opt, u'invalid-boolean')
+        # A list of strings is never recognized as a boolean
+        self.assertConvertInvalid(opt, [u'not', u'a', u'boolean'])
+
+    def test_convert_valid(self):
+        opt = self.get_option()
+        self.assertConverted(True, opt, u'True')
+        self.assertConverted(True, opt, u'1')
+        self.assertConverted(False, opt, u'False')
+
+
+class TestOptionWithIntegerConverter(tests.TestCase, TestOptionConverterMixin):
+
+    def get_option(self):
+        return config.Option('foo', help='An integer.',
+                             from_unicode=config.int_from_store)
+
+    def test_convert_invalid(self):
+        opt = self.get_option()
+        # A string that is not recognized as an integer
+        self.assertConvertInvalid(opt, u'forty-two')
+        # A list of strings is never recognized as an integer
+        self.assertConvertInvalid(opt, [u'a', u'list'])
+
+    def test_convert_valid(self):
+        opt = self.get_option()
+        self.assertConverted(16, opt, u'16')
+
+class TestOptionWithListConverter(tests.TestCase, TestOptionConverterMixin):
+
+    def get_option(self):
+        return config.Option('foo', help='A list.',
+                             from_unicode=config.list_from_store)
+
+    def test_convert_invalid(self):
+        # No string is invalid as all forms can be converted to a list
+        pass
+
+    def test_convert_valid(self):
+        opt = self.get_option()
+        # An empty string is an empty list
+        self.assertConverted([], opt, '') # Using a bare str() just in case
+        self.assertConverted([], opt, u'')
+        # A boolean
+        self.assertConverted([u'True'], opt, u'True')
+        # An integer
+        self.assertConverted([u'42'], opt, u'42')
+        # A single string
+        self.assertConverted([u'bar'], opt, u'bar')
+        # A list remains a list (configObj will turn a string containing commas
+        # into a list, but that's not what we're testing here)
+        self.assertConverted([u'foo', u'1', u'True'],
+                             opt, [u'foo', u'1', u'True'])
+
+
 class TestOptionRegistry(tests.TestCase):
 
     def setUp(self):
@@ -3045,92 +3138,80 @@
         self.overrideAttr(config, 'option_registry', config.OptionRegistry())
         self.registry = config.option_registry
 
-    def register_bool_option(self, name, default, invalid=None):
-        b = config.Option(name, default=default, help='A boolean.',
-                          from_unicode=config.bool_from_store,
-                          invalid=invalid)
+    def register_bool_option(self, name, default=None, default_from_env=None):
+        b = config.Option(name, help='A boolean.',
+                          default=default, default_from_env=default_from_env,
+                          from_unicode=config.bool_from_store)
         self.registry.register(b)
 
-    def test_get_with_bool_not_defined_default_true(self):
-        self.register_bool_option('foo', True)
-        self.assertEquals(True, self.conf.get('foo'))
-
-    def test_get_with_bool_not_defined_default_false(self):
-        self.register_bool_option('foo', False)
-        self.assertEquals(False, self.conf.get('foo'))
-
-    def test_get_with_bool_converter_not_default(self):
-        self.register_bool_option('foo', False)
-        self.conf.store._load_from_string('foo=yes')
-        self.assertEquals(True, self.conf.get('foo'))
-
-    def test_get_with_bool_converter_invalid_string(self):
-        self.register_bool_option('foo', False)
-        self.conf.store._load_from_string('foo=not-a-boolean')
-        self.assertEquals(False, self.conf.get('foo'))
-
-    def test_get_with_bool_converter_invalid_list(self):
-        self.register_bool_option('foo', False)
-        self.conf.store._load_from_string('foo=not,a,boolean')
-        self.assertEquals(False, self.conf.get('foo'))
-
-    def test_get_invalid_warns(self):
-        self.register_bool_option('foo', False, invalid='warning')
-        self.conf.store._load_from_string('foo=not-a-boolean')
-        warnings = []
-        def warning(*args):
-            warnings.append(args[0] % args[1:])
-        self.overrideAttr(trace, 'warning', warning)
-        self.assertEquals(False, self.conf.get('foo'))
-        self.assertLength(1, warnings)
-        self.assertEquals('Value "not-a-boolean" is not valid for "foo"',
-                          warnings[0])
-
-    def test_get_invalid_errors(self):
-        self.register_bool_option('foo', False, invalid='error')
-        self.conf.store._load_from_string('foo=not-a-boolean')
-        self.assertRaises(errors.ConfigOptionValueError, self.conf.get, 'foo')
-
-    def register_integer_option(self, name, default):
-        i = config.Option(name, default=default, help='An integer.',
+    def test_get_default_bool_None(self):
+        self.register_bool_option('foo')
+        self.assertEquals(None, self.conf.get('foo'))
+
+    def test_get_default_bool_True(self):
+        self.register_bool_option('foo', u'True')
+        self.assertEquals(True, self.conf.get('foo'))
+
+    def test_get_default_bool_False(self):
+        self.register_bool_option('foo', u'False')
+        self.assertEquals(False, self.conf.get('foo'))
+
+    def test_get_default_bool_False(self):
+        self.register_bool_option('foo', u'False')
+        self.assertEquals(False, self.conf.get('foo'))
+
+    def test_get_default_bool_from_env_converted(self):
+        self.register_bool_option('foo', u'True', default_from_env=['FOO'])
+        self.overrideEnv('FOO', 'False')
+        self.assertEquals(False, self.conf.get('foo'))
+
+    def test_get_default_bool_when_conversion_fails(self):
+        self.register_bool_option('foo', default='True')
+        self.conf.store._load_from_string('foo=invalid boolean')
+        self.assertEquals(True, self.conf.get('foo'))
+
+    def register_integer_option(self, name,
+                                default=None, default_from_env=None):
+        i = config.Option(name, help='An integer.',
+                          default=default, default_from_env=default_from_env,
                           from_unicode=config.int_from_store)
         self.registry.register(i)
 
-    def test_get_with_integer_not_defined_returns_default(self):
-        self.register_integer_option('foo', 42)
+    def test_get_default_integer_None(self):
+        self.register_integer_option('foo')
+        self.assertEquals(None, self.conf.get('foo'))
+
+    def test_get_default_integer(self):
+        self.register_integer_option('foo', u'42')
         self.assertEquals(42, self.conf.get('foo'))
 
-    def test_get_with_integer_converter_not_default(self):
-        self.register_integer_option('foo', 42)
-        self.conf.store._load_from_string('foo=16')
-        self.assertEquals(16, self.conf.get('foo'))
-
-    def test_get_with_integer_converter_invalid_string(self):
-        # We don't set a default value
-        self.register_integer_option('foo', None)
-        self.conf.store._load_from_string('foo=forty-two')
-        # No default value, so we should get None
-        self.assertEquals(None, self.conf.get('foo'))
-
-    def test_get_with_integer_converter_invalid_list(self):
-        # We don't set a default value
-        self.register_integer_option('foo', None)
-        self.conf.store._load_from_string('foo=a,list')
-        # No default value, so we should get None
-        self.assertEquals(None, self.conf.get('foo'))
-
-    def register_list_option(self, name, default):
-        l = config.Option(name, default=default, help='A list.',
+    def test_get_default_integer_from_env(self):
+        self.register_integer_option('foo', default_from_env=['FOO'])
+        self.overrideEnv('FOO', '18')
+        self.assertEquals(18, self.conf.get('foo'))
+
+    def test_get_default_integer_when_conversion_fails(self):
+        self.register_integer_option('foo', default='12')
+        self.conf.store._load_from_string('foo=invalid integer')
+        self.assertEquals(12, self.conf.get('foo'))
+
+    def register_list_option(self, name, default=None, default_from_env=None):
+        l = config.Option(name, help='A list.',
+                          default=default, default_from_env=default_from_env,
                           from_unicode=config.list_from_store)
         self.registry.register(l)
 
-    def test_get_with_list_not_defined_returns_default(self):
-        self.register_list_option('foo', [])
+    def test_get_default_list_None(self):
+        self.register_list_option('foo')
+        self.assertEquals(None, self.conf.get('foo'))
+
+    def test_get_default_list_empty(self):
+        self.register_list_option('foo', '')
         self.assertEquals([], self.conf.get('foo'))
 
-    def test_get_with_list_converter_nothing(self):
-        self.register_list_option('foo', [1])
-        self.conf.store._load_from_string('foo=')
+    def test_get_default_list_from_env(self):
+        self.register_list_option('foo', default_from_env=['FOO'])
+        self.overrideEnv('FOO', '')
         self.assertEquals([], self.conf.get('foo'))
 
     def test_get_with_list_converter_no_item(self):
@@ -3138,24 +3219,6 @@
         self.conf.store._load_from_string('foo=,')
         self.assertEquals([], self.conf.get('foo'))
 
-    def test_get_with_list_converter_one_boolean(self):
-        self.register_list_option('foo', [1])
-        self.conf.store._load_from_string('foo=True')
-        # We get a list of one string
-        self.assertEquals(['True'], self.conf.get('foo'))
-
-    def test_get_with_list_converter_one_integer(self):
-        self.register_list_option('foo', [1])
-        self.conf.store._load_from_string('foo=2')
-        # We get a list of one string
-        self.assertEquals(['2'], self.conf.get('foo'))
-
-    def test_get_with_list_converter_one_string(self):
-        self.register_list_option('foo', ['foo'])
-        self.conf.store._load_from_string('foo=bar')
-        # We get a list of one string
-        self.assertEquals(['bar'], self.conf.get('foo'))
-
     def test_get_with_list_converter_many_items(self):
         self.register_list_option('foo', [1])
         self.conf.store._load_from_string('foo=m,o,r,e')



More information about the bazaar-commits mailing list