Issue #22: Reviewer's suggestions.

boonya · boonya · commit 4b9089ab2ae3 · 2016-02-14T23:19:04.000+02:00
diff --git a/domain_models/fields.py b/domain_models/fields.py
@@ -37,82 +37,90 @@ def bind_model_cls(self, model_cls):
                                'could not be rebound to "{2}"'.format(
                                    self, self.model_cls, model_cls))
         self.model_cls = model_cls
-        return self.name, self
+        return self
 
     def init_model(self, model, value):
         """Init model with field."""
         if value is None and self.default is not None:
             value = self.default() if callable(self.default) else self.default
-            value = self.converter(value)
+            value = self._converter(value)
 
         if value is None and self.required:
             raise AttributeError("This field is required.")
 
         setattr(model, self.storage_name, value)
 
-    def get_value(self, model):
-        """Return field's value."""
-        return getattr(model, self.storage_name)
+    def get_value(self, model, default=None):
+        """Return field's value.
+
+        :param DomainModel model:
+        :param mixed default:
+        """
+        if default is not None:
+            default = self._converter(default)
+
+        value = getattr(model, self.storage_name)
+        return value if value is not None else default
 
     def set_value(self, model, value):
         """Set field's value."""
         if value is None and self.required:
             raise AttributeError("This field is required.")
 
         if value is not None:
-            value = self.converter(value)
+            value = self._converter(value)
 
         setattr(model, self.storage_name, value)
 
-    def converter(self, value):
+    def _converter(self, value):
         """Convert raw input value of the field."""
         return value
 
 
 class Bool(Field):
     """Bool field."""
 
-    def converter(self, value):
+    def _converter(self, value):
         """Convert raw input value of the field."""
         return bool(value)
 
 
 class Int(Field):
     """Int field."""
 
-    def converter(self, value):
+    def _converter(self, value):
         """Convert raw input value of the field."""
         return int(value)
 
 
 class Float(Field):
     """Float field."""
 
-    def converter(self, value):
+    def _converter(self, value):
         """Convert raw input value of the field."""
         return float(value)
 
 
 class String(Field):
     """String field."""
 
-    def converter(self, value):
+    def _converter(self, value):
         """Convert raw input value of the field."""
         return str(value)
 
 
 class Binary(Field):
     """Binary field."""
 
-    def converter(self, value):
+    def _converter(self, value):
         """Convert raw input value of the field."""
         return six.binary_type(value)
 
 
 class Date(Field):
     """Date field."""
 
-    def converter(self, value):
+    def _converter(self, value):
         """Convert raw input value of the field."""
         if not isinstance(value, datetime.date):
             raise TypeError('{0} is not valid date'.format(value))
@@ -122,7 +130,7 @@ def converter(self, value):
 class DateTime(Field):
     """Date and time field."""
 
-    def converter(self, value):
+    def _converter(self, value):
         """Convert raw input value of the field."""
         if not isinstance(value, datetime.datetime):
             raise TypeError('{0} is not valid date and time')
@@ -138,7 +146,7 @@ def __init__(self, related_model_cls, default=None, required=False):
 
         self.related_model_cls = related_model_cls
 
-    def converter(self, value):
+    def _converter(self, value):
         """Convert raw input value of the field."""
         if not isinstance(value, self.related_model_cls):
             raise TypeError('{0} is not valid model instance, instance of '
@@ -155,7 +163,7 @@ def __init__(self, related_model_cls, default=None, required=False):
         super(Collection, self).__init__(default=default, required=required)
         self.related_model_cls = related_model_cls
 
-    def converter(self, value):
+    def _converter(self, value):
         """Convert raw input value of the field."""
         if type(value) is not self.related_model_cls.Collection:
             value = self.related_model_cls.Collection(value)
diff --git a/domain_models/models.py b/domain_models/models.py
@@ -63,7 +63,8 @@ def prepare_fields_attribute(attribute_name, attributes, class_name):
     @staticmethod
     def bind_fields_to_model_cls(cls, model_fields):
         """Bind fields to model class."""
-        return dict(field.bind_model_cls(cls) for field in model_fields)
+        return dict(
+            (field.name, field.bind_model_cls(cls)) for field in model_fields)
 
     @staticmethod
     def bind_collection_to_model_cls(cls):
@@ -91,9 +92,9 @@ class DomainModel(object):
 
     .. py:attribute:: __fields__
 
-        Tuple of all model fields.
+        Dictionary of all model fields.
 
-        :type: tuple[fields.Field]
+        :type: dict[str, fields.Field]
 
     .. py:attribute:: __unique_key__
 
@@ -194,24 +195,17 @@ def get(self, field_name, default=None):
         one of the existent fields, the result is the value of that field.
         For example, `model.get('foobar')` is equivalent to `model.foobar`.
         If the filed does not have a value, `default` is returned if provided.
-        It will raise `AttributeError` if `default` can not be converted to
-        right type value.
+        It will raise `TypeError` or `ValueError` if `default` can not be
+        converted to right type value.
         If the field does not exist, `AttributeError` is raised as well.
 
         :param string field_name:
         :param mixed default:
         """
-        if not hasattr(self, field_name):
+        try:
+            field = self.__class__.__fields__[field_name]
+        except KeyError:
             raise AttributeError(
-                "Model doesn't have a field '{name}'".format(name=field_name))
-
-        if default is not None:
-            try:
-                field = self.__class__.__fields__[field_name]
-                default = field.converter(default)
-            except (TypeError, ValueError):
-                raise AttributeError(
-                    "default can not be converted to right type value")
-
-        value = getattr(self, field_name)
-        return value if value is not None else default
+                "Field {0} does not exist.".format(field_name))
+        else:
+            return field.get_value(self, default)
diff --git a/tests/test_models.py b/tests/test_models.py
@@ -198,9 +198,12 @@ class Model(models.DomainModel):
             self.assertEqual(model.get('field', default=value), int(value))
 
         for value in ['', u'baz']:
-            with self.assertRaises(AttributeError):
+            with self.assertRaises(ValueError):
                 model.get('field', value)
-                self.fail("Failed with {value}".format(value=str(value)))
+                self.fail("Failed with {0}".format(value))
+
+        with self.assertRaises(TypeError):
+            model.get('field', object())
 
     def test_get_method_on_string(self):
         """Test method get on String of Model."""
@@ -257,10 +260,13 @@ class Model(models.DomainModel):
         for value in valid_defaults:
             self.assertEqual(model.get('field', default=value), float(value))
 
-        for value in ['', 'baz', u'baz', datetime]:
-            with self.assertRaises(AttributeError):
+        for value in ['', 'baz', u'baz']:
+            with self.assertRaises(ValueError):
                 model.get('field', value)
-                self.fail("Failed with {value}".format(value=str(value)))
+                self.fail("Failed with {0}".format(value))
+
+        with self.assertRaises(TypeError):
+            model.get('field', object())
 
     def test_get_method_on_date(self):
         """Test method get on Date of Model."""
@@ -280,9 +286,9 @@ class Model(models.DomainModel):
         self.assertEqual(model.get('field', once), once)
 
         for value in ['', 'baz', u'baz', 0, 3, 0.7, '.5', False, True]:
-            with self.assertRaises(AttributeError):
+            with self.assertRaises(TypeError):
                 model.get('field', value)
-                self.fail("Failed with {value}".format(value=str(value)))
+                self.fail("Failed with {0}".format(value))
 
     def test_get_method_on_datetime(self):
         """Test method get on Date of Model."""
@@ -302,9 +308,9 @@ class Model(models.DomainModel):
         self.assertEqual(model.get('field', once), once)
 
         for value in ['', 'baz', u'baz', 0, 3, 0.7, '.5', False, True]:
-            with self.assertRaises(AttributeError):
+            with self.assertRaises(TypeError):
                 model.get('field', value)
-                self.fail("Failed with {value}".format(value=str(value)))
+                self.fail("Failed with {0}".format(value))
 
     def test_get_method_on_binary(self):
         """Test method get on Binary of Model."""
