configglue team mailing list archive

[Ricardo Kirkner <ricardo.kirkner@xxxxxxxxxxxxx>]

Ricardo Kirkner has proposed merging lp:~ricardokirkner/configglue/793060-set-nonstring into lp:configglue.

Requested reviews:
  Configglue developers (configglue)
Related bugs:
  Bug #793060 in configglue: "TypeError: option values must be strings"
  https://bugs.launchpad.net/configglue/+bug/793060

For more details, see:
https://code.launchpad.net/~ricardokirkner/configglue/793060-set-nonstring/+merge/63772

This branch updates the parser code so that the set method will always take a valid value for the associated ConfigOption object, but will internally still cast it to a string for storage to keep compatible with SafeConfigParser.
-- 
https://code.launchpad.net/~ricardokirkner/configglue/793060-set-nonstring/+merge/63772
Your team Configglue developers is requested to review the proposed merge of lp:~ricardokirkner/configglue/793060-set-nonstring into lp:configglue.

=== modified file 'configglue/pyschema/glue.py'
--- configglue/pyschema/glue.py	2011-03-26 22:24:51 +0000
+++ configglue/pyschema/glue.py	2011-06-07 21:34:26 +0000
@@ -71,11 +71,10 @@
         for option in section.options():
             value = getattr(options, opt_name(option))
             if parser.get(section.name, option.name) != value:
-                # the value has been overridden by an argument;
-                # update it, but make sure it's a string, as
-                # SafeConfigParser will complain otherwise.
-                if not isinstance(value, basestring):
-                    value = repr(value)
+                # the value has been overridden by an argument
+                if isinstance(value, basestring):
+                    # parse it to the right type if it's a string
+                    value = option.parse(value)
                 parser.set(section.name, option.name, value)
 
     return op, options, args
@@ -101,4 +100,3 @@
     if not is_valid:
         parser.error(reasons[0])
     return SchemaGlue(scp, parser, opts, args)
-

=== modified file 'configglue/pyschema/parser.py'
--- configglue/pyschema/parser.py	2011-05-02 16:23:04 +0000
+++ configglue/pyschema/parser.py	2011-06-07 21:34:26 +0000
@@ -349,7 +349,7 @@
 
             if is_default_value:
                 value = option_obj.default
-            else:
+            elif isinstance(value, basestring):
                 try:
                     value = option_obj.parse(value, **kwargs)
                 except ValueError, e:
@@ -464,9 +464,7 @@
             #    raise NoSectionError(section)
             return None
         else:
-            # we want to return a non-parsed value
-            # a unicode of the value is the closest we can get
-            return unicode(value)
+            return value
 
     def get(self, section, option, raw=False, vars=None, parse=True):
         """Return the parsed value of an option.
@@ -482,8 +480,7 @@
         try:
             # get option's raw mode setting
             try:
-                section_obj = self.schema.section(section)
-                option_obj = section_obj.option(option)
+                option_obj = self._get_option(section, option)
                 raw = option_obj.raw or raw
             except:
                 pass
@@ -515,11 +512,24 @@
             value = self.parse(section, option, value)
         return value
 
+    def _get_option(self, section, option):
+        section_obj = self.schema.section(section)
+        option_obj = section_obj.option(option)
+        return option_obj
+
     def set(self, section, option, value):
         """Set an option's raw value."""
-        super(SchemaConfigParser, self).set(section, option, value)
+        option_obj = self._get_option(section, option)
+        # make sure the value is of the right type for the option
+        if not option_obj.validate(value):
+            raise TypeError("{0} is not a valid {1} value.".format(
+                value, type(option_obj).__name__))
+        # cast value to a string because SafeConfigParser only allows
+        # strings to be set
+        str_value = option_obj.to_string(value)
+        super(SchemaConfigParser, self).set(section, option, str_value)
         filename = self.locate(option)
-        self._dirty[filename][section][option] = value
+        self._dirty[filename][section][option] = str_value
 
     def write(self, fp):
         """Write an .ini-format representation of the configuration state."""

=== modified file 'configglue/pyschema/schema.py'
--- configglue/pyschema/schema.py	2011-05-19 09:13:12 +0000
+++ configglue/pyschema/schema.py	2011-06-07 21:34:26 +0000
@@ -277,6 +277,13 @@
         """Parse the given value."""
         raise NotImplementedError()
 
+    def validate(self, value):
+        raise NotImplementedError()
+
+    def to_string(self, value):
+        """Return a string representation of the value."""
+        return str(value)
+
 
 class BoolConfigOption(ConfigOption):
     """A ConfigOption that is parsed into a bool"""
@@ -300,6 +307,9 @@
         else:
             raise ValueError("Unable to determine boolosity of %r" % value)
 
+    def validate(self, value):
+        return isinstance(value, bool)
+
 
 class IntConfigOption(ConfigOption):
     """A ConfigOption that is parsed into an int"""
@@ -318,6 +328,9 @@
 
         return int(value)
 
+    def validate(self, value):
+        return isinstance(value, int)
+
 
 class LinesConfigOption(ConfigOption):
     """A ConfigOption that is parsed into a list of objects
@@ -369,6 +382,9 @@
             items = filtered_items
         return items
 
+    def validate(self, value):
+        return isinstance(value, list)
+
 
 class StringConfigOption(ConfigOption):
     """A ConfigOption that is parsed into a string.
@@ -403,6 +419,12 @@
             result = repr(value)
         return result
 
+    def to_string(self, value):
+        return value
+
+    def validate(self, value):
+        return isinstance(value, basestring)
+
 
 class TupleConfigOption(ConfigOption):
     """A ConfigOption that is parsed into a fixed-size tuple of strings.
@@ -441,6 +463,9 @@
             # length is 0, so no length validation
         return result
 
+    def validate(self, value):
+        return isinstance(value, tuple)
+
 
 class DictConfigOption(ConfigOption):
     """A ConfigOption that is parsed into a dictionary.
@@ -520,6 +545,9 @@
                     result[key] = value
         return result
 
+    def validate(self, value):
+        return isinstance(value, dict)
+
     def get_extra_sections(self, section, parser):
         sections = []
         for option in parser.options(section):
@@ -545,4 +573,3 @@
                 sections.extend(extra)
 
         return sections
-

=== modified file 'tests/pyschema/test_parser.py'
--- tests/pyschema/test_parser.py	2011-05-19 08:44:58 +0000
+++ tests/pyschema/test_parser.py	2011-06-07 21:34:26 +0000
@@ -597,7 +597,7 @@
             class foo(ConfigSection):
                 bar = IntConfigOption()
         config = StringIO("[__main__]\n")
-        expected = '0'
+        expected = 0
 
         parser = SchemaConfigParser(MySchema())
         parser.readfp(config)
@@ -721,6 +721,24 @@
             self.assertEqual(self.parser._dirty,
                 {f.name: {'__main__': {'foo': '2'}}})
 
+    def test_set_non_string(self):
+        class MySchema(Schema):
+            foo = IntConfigOption()
+            bar = BoolConfigOption()
+        parser = SchemaConfigParser(MySchema())
+        parser.parse_all()
+
+        parser.set('__main__', 'foo', 2)
+        parser.set('__main__', 'bar', False)
+        self.assertEqual(parser.get('__main__', 'foo'), 2)
+        self.assertEqual(parser._sections['__main__']['foo'], '2')
+        self.assertEqual(parser.get('__main__', 'bar'), False)
+        self.assertEqual(parser._sections['__main__']['bar'], 'False')
+
+    def test_set_invalid_type(self):
+        self.parser.parse_all()
+        self.assertRaises(TypeError, self.parser.set, '__main__', 'foo', 2)
+
     def test_write(self):
         class MySchema(Schema):
             foo = StringConfigOption()

=== modified file 'tests/pyschema/test_schema.py'
--- tests/pyschema/test_schema.py	2011-05-27 19:17:15 +0000
+++ tests/pyschema/test_schema.py	2011-06-07 21:34:26 +0000
@@ -22,6 +22,7 @@
 from configglue.pyschema.parser import SchemaConfigParser
 from configglue.pyschema.schema import (
     BoolConfigOption,
+    ConfigOption,
     ConfigSection,
     DictConfigOption,
     IntConfigOption,
@@ -168,6 +169,10 @@
         del opt2.name
         self.assertNotEqual(opt1, opt2)
 
+    def test_validate(self):
+        opt = ConfigOption()
+        self.assertRaises(NotImplementedError, opt.validate, 0)
+
 
 class TestSchemaInheritance(unittest.TestCase):
     def setUp(self):
@@ -287,6 +292,14 @@
         opt = StringConfigOption(null=True)
         self.assertEqual(opt.default, None)
 
+    def test_validate_string(self):
+        opt = StringConfigOption()
+        self.assertEqual(opt.validate(''), True)
+
+    def test_validate_nonstring(self):
+        opt = StringConfigOption()
+        self.assertEqual(opt.validate(0), False)
+
 
 class TestIntConfigOption(unittest.TestCase):
     def test_parse_int(self):
@@ -313,6 +326,14 @@
         opt = IntConfigOption()
         self.assertEqual(opt.default, 0)
 
+    def test_validate_int(self):
+        opt = IntConfigOption()
+        self.assertEqual(opt.validate(0), True)
+
+    def test_validate_nonint(self):
+        opt = IntConfigOption()
+        self.assertEqual(opt.validate(''), False)
+
 
 class TestBoolConfigOption(unittest.TestCase):
     def test_parse_bool(self):
@@ -344,6 +365,14 @@
         opt = BoolConfigOption()
         self.assertEqual(opt.default, False)
 
+    def test_validate_bool(self):
+        opt = BoolConfigOption()
+        self.assertEqual(opt.validate(False), True)
+
+    def test_validate_nonbool(self):
+        opt = BoolConfigOption()
+        self.assertEqual(opt.validate(''), False)
+
 
 class TestLinesConfigOption(unittest.TestCase):
     def test_parse_int_lines(self):
@@ -417,6 +446,15 @@
         self.assertEquals({'__main__': {'foo': [{'bar': 'baz'}]}},
                           parser.values())
 
+    def test_validate_list(self):
+        opt = LinesConfigOption(item=IntConfigOption())
+        self.assertEqual(opt.validate([]), True)
+
+    def test_validate_nonlist(self):
+        opt = LinesConfigOption(item=IntConfigOption())
+        self.assertEqual(opt.validate(''), False)
+
+
 class TestTupleConfigOption(unittest.TestCase):
     def test_init(self):
         opt = TupleConfigOption(length=2)
@@ -461,6 +499,14 @@
         opt = TupleConfigOption(length=2)
         self.assertEqual(opt.default, ())
 
+    def test_validate_tuple(self):
+        opt = TupleConfigOption(length=2)
+        self.assertEqual(opt.validate(()), True)
+
+    def test_validate_nontuple(self):
+        opt = TupleConfigOption(length=2)
+        self.assertEqual(opt.validate(0), False)
+
 
 class TestDictConfigOption(unittest.TestCase):
     def test_init(self):
@@ -638,6 +684,14 @@
         parser.readfp(config)
         self.assertRaises(ValueError, parser.parse_all)
 
+    def test_validate_dict(self):
+        opt = DictConfigOption()
+        self.assertEqual(opt.validate({}), True)
+
+    def test_validate_nondict(self):
+        opt = DictConfigOption()
+        self.assertEqual(opt.validate(0), False)
+
 
 class TestLinesOfDictConfigOption(unittest.TestCase):
     def test_parse_lines_of_dict(self):