Merge lp:~hazmat/pyjuju/bool-and-validate-defaults into lp:pyjuju

[Kapil Thangavelu]

Validate defaults and add a boolean config type

Defaults previously could be inconsistent to the schema without detection. Per
request also implement a boolean configuration type.

https://codereview.appspot.com/5572053/

[Kapil Thangavelu (hazmat)]

Reviewers: mp+89978_code.launchpad.net,

Message:
Please take a look.

Description:
Defaults previously could be inconsistent to the schema without
detection. Per
request also implement a boolean configuration type.

https://code.launchpad.net/~hazmat/juju/bool-and-validate-defaults/+merge/89978

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/5572053/

Affected files:
   M juju/charm/config.py
   M juju/charm/tests/test_config.py

Index: juju/charm/config.py
=== <email address hidden> >
<email address hidden>
=== modified file 'juju/charm/config.py'
--- juju/charm/config.py 2011-12-07 02:32:07 +0000
+++ juju/charm/config.py 2012-01-24 18:10:19 +0000
@@ -13,6 +13,7 @@
          "type": OneOf(Constant("string"),
                        Constant("str"), # Obsolete
                        Constant("int"),
+ Constant("boolean"),
                        Constant("float")),
          "default": OneOf(String(), Int(), Float()),
          "description": String(),
@@ -91,6 +92,8 @@

          data = self.parse_serialization_data(raw_data, pathname)
          self._data = data
+ # validate defaults
+ self.get_defaults()

      def parse_serialization_data(self, data, pathname=None):
          """Verify we have sensible option metadata.
@@ -136,7 +139,7 @@

          if not valid:
              raise ServiceConfigValueError(
- "Invalid value for %s: %s" % (name, value))
+ "Invalid value for %s: %r" % (name, value))
          return value

      def get_defaults(self):
@@ -173,7 +176,9 @@

  # Validators return (type mapped value, valid boolean)
  def validate_str(value, options):
- return str(value), True
+ if isinstance(value, basestring):
+ return value, True
+ return value, False

  def validate_int(value, options):
@@ -184,10 +189,15 @@

  def validate_float(value, options):
- try:
+ if isinstance(value, (int, float)):
          return float(value), True
- except ValueError:
- return value, False
+ return value, False
+
+
+def validate_boolean(value, options):
+ if isinstance(value, bool):
+ return value, True
+ return value, False

  # maps service option types to callables
  validation_kinds = {
@@ -195,4 +205,5 @@
      "str": validate_str, # Obsolete
      "int": validate_int,
      "float": validate_float,
+ "boolean": validate_boolean,
      }

Index: juju/charm/tests/test_config.py
=== <email address hidden> >
<email address hidden>
=== modified file 'juju/charm/tests/test_config.py'
--- juju/charm/tests/test_config.py 2011-12-07 02:32:07 +0000
+++ juju/charm/tests/test_config.py 2012-01-24 18:42:22 +0000
@@ -78,6 +78,19 @@
          defaults = self.config.get_defaults()
          self.assertEqual(defaults, sample_config_defaults)

+ def test_defaults_validated(self):
+ e = self.assertRaises(
+ ServiceConfigValueError,
+ self.config.parse,
+ yaml.dump(
+ ...

[Jim Baker (jimbaker)]

+1, another easy & small branch to review!

review: Approve

[Benjamin Saller (bcsaller)]

lgtm

review: Approve

=== modified file 'juju/charm/config.py'
--- juju/charm/config.py	2011-12-07 02:32:07 +0000
+++ juju/charm/config.py	2012-01-24 19:02:27 +0000
@@ -13,6 +13,7 @@
         "type": OneOf(Constant("string"),
                       Constant("str"),  # Obsolete
                       Constant("int"),
+                      Constant("boolean"),
                       Constant("float")),
         "default": OneOf(String(), Int(), Float()),
         "description": String(),
@@ -91,6 +92,8 @@
 
         data = self.parse_serialization_data(raw_data, pathname)
         self._data = data
+        # validate defaults
+        self.get_defaults()
 
     def parse_serialization_data(self, data, pathname=None):
         """Verify we have sensible option metadata.
@@ -136,7 +139,7 @@
 
         if not valid:
             raise ServiceConfigValueError(
-                "Invalid value for %s: %s" % (name, value))
+                "Invalid value for %s: %r" % (name, value))
         return value
 
     def get_defaults(self):
@@ -173,7 +176,9 @@
 
 # Validators return (type mapped value, valid boolean)
 def validate_str(value, options):
-    return str(value), True
+    if isinstance(value, basestring):
+        return value, True
+    return value, False
 
 
 def validate_int(value, options):
@@ -184,10 +189,15 @@
 
 
 def validate_float(value, options):
-    try:
+    if isinstance(value, (int, float)):
         return float(value), True
-    except ValueError:
-        return value, False
+    return value, False
+
+
+def validate_boolean(value, options):
+    if isinstance(value, bool):
+        return value, True
+    return value, False
 
 # maps service option types to callables
 validation_kinds = {
@@ -195,4 +205,5 @@
     "str": validate_str,  # Obsolete
     "int": validate_int,
     "float": validate_float,
+    "boolean": validate_boolean,
     }

=== modified file 'juju/charm/tests/test_config.py'
--- juju/charm/tests/test_config.py	2011-12-07 02:32:07 +0000
+++ juju/charm/tests/test_config.py	2012-01-24 19:02:27 +0000
@@ -78,6 +78,19 @@
         defaults = self.config.get_defaults()
         self.assertEqual(defaults, sample_config_defaults)
 
+    def test_defaults_validated(self):
+        e = self.assertRaises(
+            ServiceConfigValueError,
+            self.config.parse,
+            yaml.dump(
+                {"options": {
+                    "foobar": {
+                        "description": "beyond what?",
+                        "type": "string",
+                        "default": True}}}))
+        self.assertEqual(
+            str(e), "Invalid value for foobar: True")
+
     def test_as_dict(self):
         # load valid data
         filename = self.makeFile(sample_configuration)
@@ -129,12 +142,49 @@
         self.assertRaises(
             ServiceConfigValueError, config.validate, sample_input)
 
-    def test_validate_types(self):
+    def test_validate_float(self):
+        self.config.parse(yaml.dump(
+            {"options": {
+                "score": {
+                    "description": "A number indicating score.",
+                    "type": "float"}}}))
+        error = self.assertRaises(ServiceConfigValueError,
+                                  self.config.validate, {"score": "82"})
+        self.assertEquals(str(error), "Invalid value for score: '82'")
+
+        data = self.config.validate({"score": 82})
+        self.assertEqual(data, {"score": 82})
+
+    def test_validate_string(self):
+        self.config.parse(sample_configuration)
+
+        error = self.assertRaises(ServiceConfigValueError,
+                                  self.config.validate, {"title": True})
+        self.assertEquals(str(error), "Invalid value for title: True")
+
+        data = self.config.validate({"title": u"Good"})
+        self.assertEqual(data, {"title": u"Good"})
+
+    def test_validate_boolean(self):
+        self.config.parse(yaml.dump(
+            {"options": {
+                "active": {
+                    "description": "A boolean indicating activity.",
+                    "type": "boolean"}}}))
+
+        error = self.assertRaises(ServiceConfigValueError,
+                                  self.config.validate, {"active": "True"})
+        self.assertEquals(str(error), "Invalid value for active: 'True'")
+
+        data = self.config.validate({"active": False})
+        self.assertEqual(data, {"active": False})
+
+    def test_validate_integer(self):
         self.config.parse(sample_configuration)
 
         error = self.assertRaises(ServiceConfigValueError,
                                   self.config.validate, {"skill-level": "NaN"})
-        self.assertEquals(str(error), "Invalid value for skill-level: NaN")
+        self.assertEquals(str(error), "Invalid value for skill-level: 'NaN'")
 
         data = self.config.validate({"skill-level": "9001"})
         # its over 9000!