Merge lp:~frankban/juju-quickstart/improve-unexpected-fields into lp:juju-quickstart

[Francesco Banconi]

Improve unexpected fields handling.

Implement the UnexpectedField type. This is used
to handle unexpected key/value pairs found in
the environments.yaml file. The user will be able
to get rid of those fields just deleting their
values from in the interactive session.

Tests: `make check`.
No QA.

https://codereview.appspot.com/43380043/

[Francesco Banconi (frankban)]

Reviewers: mp+199270_code.launchpad.net,

Message:
Please take a look.

Description:
Improve unexpected fields handling.

Implement the UnexpectedField type. This is used
to handle unexpected key/value pairs found in
the environments.yaml file. The user will be able
to get rid of those fields just deleting their
values from in the interactive session.

Tests: `make check`.
No QA.

https://code.launchpad.net/~frankban/juju-quickstart/improve-unexpected-fields/+merge/199270

(do not edit description out of merge proposal)

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

Affected files (+142, -3 lines):
   A [revision details]
   M quickstart/models/envs.py
   M quickstart/models/fields.py
   M quickstart/tests/models/test_envs.py
   M quickstart/tests/models/test_fields.py

[lp:~frankban/juju-quickstart/improve-unexpected-fields]

39. By Francesco Banconi on 2013-12-17

Merged trunk.

[Madison Scott-Clary (makyo)]

LGTM, thanks!

https://codereview.appspot.com/43380043/

[Francesco Banconi (frankban)]

*** Submitted:

Improve unexpected fields handling.

Implement the UnexpectedField type. This is used
to handle unexpected key/value pairs found in
the environments.yaml file. The user will be able
to get rid of those fields just deleting their
values from in the interactive session.

Tests: `make check`.
No QA.

R=matthew.scott
CC=
https://codereview.appspot.com/43380043

https://codereview.appspot.com/43380043/

[Francesco Banconi (frankban)]

Thank you!

https://codereview.appspot.com/43380043/

=== modified file 'quickstart/models/envs.py'
--- quickstart/models/envs.py	2013-12-16 17:07:07 +0000
+++ quickstart/models/envs.py	2013-12-17 16:26:53 +0000
@@ -448,10 +448,9 @@
         (field, data.pop(field.name, None))
         for field in env_metadata['fields']
     ]
-    # Add the remaining (unexpected) pairs using the base field type.
-    help = 'this field is unrecognized and can be safely removed'
+    # Add the remaining (unexpected) pairs using the unexpected field type.
     field_value_pairs.extend(
-        (fields.Field(key, help=help, required=False), value)
+        (fields.UnexpectedField(key), value)
         for key, value in data.items()
     )
     return field_value_pairs

=== modified file 'quickstart/models/fields.py'
--- quickstart/models/fields.py	2013-12-16 11:52:31 +0000
+++ quickstart/models/fields.py	2013-12-17 16:26:53 +0000
@@ -223,6 +223,47 @@
             raise ValueError(msg.encode('utf-8'))
 
 
+class UnexpectedField(Field):
+    """An unexpected field, used when the value type is unknown."""
+
+    def __init__(self, name, label=None, help=None):
+        if help is None:
+            help = 'this field is unrecognized and can be safely removed'
+        super(UnexpectedField, self).__init__(
+            name, label=label, help=help,
+            default=None, required=False, readonly=False)
+
+    def normalize(self, value):
+        """Try to guess the value type."""
+        if isinstance(value, (bool, int, type(None))):
+            # For booleans, numbers, and None values, return the value as is.
+            return value
+        if isinstance(value, unicode):
+            # If the value is a string (e.g. because it is returned as a
+            # string by view code), try to guess the underlying type.
+            value = value.strip()
+            # Check if the value is not set and can be discarded.
+            if not value:
+                return None
+            # Check if the value is a number.
+            if value.isdigit():
+                return int(value)
+            # Check if the value is a boolean.
+            lower_value = value.lower()
+            if lower_value == 'true':
+                return True
+            if lower_value == 'false':
+                return False
+            # The value is a string, return it.
+            return value
+        # If the above did not work, always turn the value into a string.
+        return unicode(value)
+
+    def validate(self, value):
+        """Unexpected values are always valid."""
+        pass
+
+
 class ChoiceField(StringField):
     """A string field whose value must be included in the given choices."""
 

=== modified file 'quickstart/tests/models/test_envs.py'
--- quickstart/tests/models/test_envs.py	2013-12-16 17:07:07 +0000
+++ quickstart/tests/models/test_envs.py	2013-12-17 16:26:53 +0000
@@ -645,6 +645,7 @@
         self.assertEqual(len(unexpected_dict), len(remaining_pairs))
         help = 'this field is unrecognized and can be safely removed'
         for field, value in remaining_pairs:
+            self.assertIsInstance(field, fields.UnexpectedField)
             self.assertEqual(unexpected_dict[field.name], value, field.name)
             self.assertFalse(field.required, field.name)
             self.assertEqual(help, field.help, field.name)
@@ -803,6 +804,30 @@
         normalized_data = envs.normalize(self.env_metadata, env_data)
         self.assertEqual(expected, normalized_data)
 
+    def test_exclude_unexpected_fields(self):
+        # The normalization process excludes unexpected fields if the
+        # corresponding values are not set.
+        env_data = {
+            # Since this field is required, it is included even if not set.
+            'string-required': 'a string',
+            # Since the value is set, this value is preserved even if the
+            # field is not included in metadata.
+            'unexpected1': 'boo!',
+            # False is also considered a valid value for an unexpected field.
+            'unexpected2': False,
+            # Unexpected fields whose value is empty or None are excluded.
+            'unexpected3': '',
+            'unexpected4': '\t\n ',
+            'unexpected5': None,
+        }
+        expected = {
+            'string-required': 'a string',
+            'unexpected1': 'boo!',
+            'unexpected2': False,
+        }
+        normalized_data = envs.normalize(self.env_metadata, env_data)
+        self.assertEqual(expected, normalized_data)
+
     def test_original_not_mutated(self):
         # The original env_data is not modified in the process.
         env_data = {

=== modified file 'quickstart/tests/models/test_fields.py'
--- quickstart/tests/models/test_fields.py	2013-12-16 12:11:15 +0000
+++ quickstart/tests/models/test_fields.py	2013-12-17 16:26:53 +0000
@@ -335,6 +335,78 @@
             field.validate(None)
 
 
+class TestUnexpectedField(FieldTestsMixin, unittest.TestCase):
+
+    field_class = fields.UnexpectedField
+
+    def assert_normalized(self, normalized_value, input_value):
+        """Ensure the expected normalized value is returned."""
+        field = self.field_class('last-name')
+        self.assertEqual(normalized_value, field.normalize(input_value))
+
+    def test_attributes(self):
+        # The field attributes are properly stored in the field instance.
+        field = self.field_class(
+            'first-name', label='first name', help='your first name')
+        self.assertEqual('first-name', field.name)
+        self.assertEqual('first name', field.label)
+        self.assertEqual('your first name', field.help)
+
+    def test_default_attributes(self):
+        # Only the name identifier is required when instantiating a field.
+        field = self.field_class('last-name')
+        self.assertEqual('last-name', field.name)
+        self.assertEqual('last-name', field.label)
+        self.assertEqual(
+            'this field is unrecognized and can be safely removed', field.help)
+        self.assertIsNone(field.default)
+        self.assertFalse(field.required)
+        self.assertFalse(field.readonly)
+
+    def test_normalize_boolean(self):
+        # The normalized boolean value is the value itself.
+        self.assert_normalized(True, True)
+        self.assert_normalized(False, False)
+
+    def test_normalize_integer(self):
+        # The normalized numeric value is the number itself.
+        self.assert_normalized(42, 42)
+
+    def test_normalize_none(self):
+        # None normalization returns None.
+        self.assert_normalized(None, None)
+
+    def test_normalize_string_guess(self):
+        # A string value is converted to the underlying type if possible.
+        self.assert_normalized(42, '42')
+        self.assert_normalized(47, ' 47 ')
+        self.assert_normalized(True, 'true')
+        self.assert_normalized(False, '\tFALSE\n')
+
+    def test_normalize_string(self):
+        # The normalization process returns the stripped value if the value is
+        # a string.
+        self.assert_normalized('a string', 'a string')
+        self.assert_normalized('stripped', '\n stripped\t')
+
+    def test_normalize_empty_string(self):
+        # An empty string is normalized to None.
+        self.assert_normalized(None, '')
+        self.assert_normalized(None, ' ')
+
+    def test_normalize_unknown(self):
+        # The normalization process converts to string unrecognized values.
+        self.assert_normalized('42.47', 42.47)
+        self.assert_normalized('{}', {})
+
+    def test_validate(self):
+        # Unexpected values are always valid.
+        field = self.field_class('last-name')
+        for value in (None, 42, 42.47, True, False, 'a string'):
+            with self.assert_not_raises(ValueError, value):
+                field.validate(value)
+
+
 class TestAutoGeneratedStringField(TestStringField):
 
     field_class = fields.AutoGeneratedStringField