On 10/08/2013 08:37 PM, Nathaniel McCallum wrote:
On Tue, 2013-10-08 at 18:29 +0200, Petr Viktorin wrote:
On 10/07/2013 11:28 PM, Nathaniel McCallum wrote:
On Mon, 2013-10-07 at 13:22 +0200, Petr Viktorin wrote:
On 10/04/2013 07:33 PM, Nathaniel McCallum wrote:
This patch is preparatory for the OTP CLI patch.



   > +    def _convert_scalar(self, value, index=None):
   > +        return Int._convert_scalar(self, value, index=index)

That won't work. In Python 2 unbound methods (such as
Int._validate_scalar) must be passed the correct type as self; passing
an IntEnum instance like this will raise a TypeError.

You'll need to either use multiple inheritance (if you feel the
framework isn't complex enough), or make a convert_int function, and
then in both Int and IntEnum just call it and handle ValueError.

For validate_scalar it would probably be best to extend
Param._validate_scalar to allow the class to define extra allowed types,
and get rid of the reimplementation in Int.

Fixed.

This works, but I do have some comments.

I'd prefer if the framework changes and IntEnum addition were in
separate patches.
I find the usage of _get_types() a bit convoluted, especially when you
need to get the "canonical" type. I've taken the liberty to do this with
an `allowed_types` attribute, which I think is easier to use, and also
doesn't break the API.
Would you agree with these changes?

I've added tests for IntEnum, as a combination of StrEnum and Int tests.
Do they look OK?

Everything looks great except I suspect the reworking of convert_int()
belongs in the second patch.

Makes sense, fixed.


--
PetrĀ³
From 70f7a88774e9b2f1f148bfaeee2635d966c38342 Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum <npmccal...@redhat.com>
Date: Mon, 30 Sep 2013 12:45:37 -0400
Subject: [PATCH] Allow multiple types in Param type validation

Int already needed to take both int and long. This makes the functionality
available for all Param classes.
---
 ipalib/parameters.py                    | 53 +++++++++++----------------------
 ipatests/test_ipalib/test_parameters.py |  3 +-
 2 files changed, 20 insertions(+), 36 deletions(-)

diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index 79b9062bb52c639ce72bc7d7fae19e7fba100e51..97e449c29902eb3ee1c2213a16a281fdc6ec62fc 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -363,7 +363,9 @@ class Param(ReadOnly):
 
     # This is a dummy type so that most of the functionality of Param can be
     # unit tested directly without always creating a subclass; however, a real
-    # (direct) subclass must *always* override this class attribute:
+    # (direct) subclass must *always* override this class attribute.
+    # If multiple types are permitted, set `type` to the canonical type and
+    # `allowed_types` to a tuple of all allowed types.
     type = NoneType  # Ouch, this wont be very useful in the real world!
 
     # Subclasses should override this with something more specific:
@@ -400,6 +402,11 @@ class Param(ReadOnly):
         # ('default', self.type, None),
     )
 
+    @property
+    def allowed_types(self):
+        """The allowed datatypes for this Param"""
+        return (self.type,)
+
     def __init__(self, name, *rules, **kw):
         # We keep these values to use in __repr__():
         self.param_spec = name
@@ -415,7 +422,7 @@ def __init__(self, name, *rules, **kw):
         self.nice = '%s(%r)' % (self.__class__.__name__, self.param_spec)
 
         # Add 'default' to self.kwargs and makes sure no unknown kw were given:
-        assert type(self.type) is type
+        assert all(type(t) is type for t in self.allowed_types)
         if kw.get('multivalue', True):
             self.kwargs += (('default', tuple, None),)
         else:
@@ -782,7 +789,7 @@ def _convert_scalar(self, value, index=None):
         """
         Convert a single scalar value.
         """
-        if type(value) is self.type:
+        if type(value) in self.allowed_types:
             return value
         raise ConversionError(name=self.name, index=index,
             error=ugettext(self.type_error),
@@ -816,7 +823,7 @@ def validate(self, value, context=None, supplied=None):
             self._validate_scalar(value)
 
     def _validate_scalar(self, value, index=None):
-        if type(value) is not self.type:
+        if type(value) not in self.allowed_types:
             raise TypeError(
                 TYPE_ERROR % (self.name, self.type, value, type(value))
             )
@@ -942,7 +949,7 @@ def _convert_scalar(self, value, index=None):
         """
         Convert a single scalar value.
         """
-        if type(value) is self.type:
+        if type(value) in self.allowed_types:
             return value
         if isinstance(value, basestring):
             value = value.lower()
@@ -1009,7 +1016,7 @@ def _convert_scalar(self, value, index=None):
         """
         Convert a single scalar value.
         """
-        if type(value) is self.type:
+        if type(value) in self.allowed_types:
             return value
         if type(value) in (unicode, int, long, float):
             try:
@@ -1030,6 +1037,7 @@ class Int(Number):
     """
 
     type = int
+    allowed_types = int, long
     type_error = _('must be an integer')
 
     kwargs = Param.kwargs + (
@@ -1095,31 +1103,6 @@ def _rule_maxvalue(self, _, value):
                 maxvalue=self.maxvalue,
             )
 
-    def _validate_scalar(self, value, index=None):
-        """
-        This duplicates _validate_scalar in the Param class with
-        the exception that it allows both int and long types. The
-        min/max rules handle size enforcement.
-        """
-        if type(value) not in (int, long):
-            raise TypeError(
-                TYPE_ERROR % (self.name, self.type, value, type(value))
-            )
-        if index is not None and type(index) is not int:
-            raise TypeError(
-                TYPE_ERROR % ('index', int, index, type(index))
-            )
-        for rule in self.all_rules:
-            error = rule(ugettext, value)
-            if error is not None:
-                raise ValidationError(
-                    name=self.get_param_name(),
-                    value=value,
-                    index=index,
-                    error=error,
-                    rule=rule,
-                )
-
 
 class Decimal(Number):
     """
@@ -1315,7 +1298,7 @@ def _rule_pattern(self, _, value):
         """
         Check pattern (regex) contraint.
         """
-        assert type(value) is self.type
+        assert type(value) in self.allowed_types
         if self.re.match(value) is None:
             if self.re_errmsg:
                 return self.re_errmsg % dict(pattern=self.pattern,)
@@ -1418,7 +1401,7 @@ def _convert_scalar(self, value, index=None):
         """
         Convert a single scalar value.
         """
-        if type(value) is self.type:
+        if type(value) in self.allowed_types:
             return value
         if type(value) in (int, long, float, decimal.Decimal):
             return self.type(value)
@@ -1522,7 +1505,7 @@ class Enum(Param):
     def __init__(self, name, *rules, **kw):
         super(Enum, self).__init__(name, *rules, **kw)
         for (i, v) in enumerate(self.values):
-            if type(v) is not self.type:
+            if type(v) not in self.allowed_types:
                 n = '%s values[%d]' % (self.nice, i)
                 raise TypeError(
                     TYPE_ERROR % (n, self.type, v, type(v))
@@ -1789,7 +1772,7 @@ def _convert_scalar(self, value, index=None):
         """
         Convert a single scalar value.
         """
-        if type(value) is self.type:
+        if type(value) in self.allowed_types:
             return value
 
         try:
diff --git a/ipatests/test_ipalib/test_parameters.py b/ipatests/test_ipalib/test_parameters.py
index 71acfce71b52c24a4bb80b846d9d2f3e3313574b..22c7b7355d60570aa03285562f0b9e6ddf4932d6 100644
--- a/ipatests/test_ipalib/test_parameters.py
+++ b/ipatests/test_ipalib/test_parameters.py
@@ -1173,7 +1173,8 @@ def test_init(self):
         """
         # Test with no kwargs:
         o = self.cls('my_number')
-        assert o.type is int
+        assert o.type == int
+        assert o.allowed_types == (int, long)
         assert isinstance(o, parameters.Int)
         assert o.minvalue == int(MININT)
         assert o.maxvalue == int(MAXINT)
-- 
1.8.3.1

From 665668cbca192dab361cdd6f81aff378669a7cae Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum <npmccal...@redhat.com>
Date: Wed, 9 Oct 2013 10:16:46 +0200
Subject: [PATCH] Add IntEnum parameter to ipalib

---
 ipalib/__init__.py   |  2 +-
 ipalib/parameters.py | 66 +++++++++++++++++++++++++++++++++-------------------
 2 files changed, 43 insertions(+), 25 deletions(-)

diff --git a/ipalib/__init__.py b/ipalib/__init__.py
index d822ba5956d6afb6ef6d88063f8359926e47016b..ab89ab77ec94603d242e56436021c9b6ed8663cb 100644
--- a/ipalib/__init__.py
+++ b/ipalib/__init__.py
@@ -886,7 +886,7 @@ class my_command(Command):
 from frontend import Object, Method, Property
 from crud import Create, Retrieve, Update, Delete, Search
 from parameters import DefaultFrom, Bool, Flag, Int, Decimal, Bytes, Str, IA5Str, Password, DNParam, DeprecatedParam
-from parameters import BytesEnum, StrEnum, AccessTime, File
+from parameters import BytesEnum, StrEnum, IntEnum, AccessTime, File
 from errors import SkipPluginModule
 from text import _, ngettext, GettextFactory, NGettextFactory
 
diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index 97e449c29902eb3ee1c2213a16a281fdc6ec62fc..6e342f96567f9e9c5dbe26600dd23a26345e67e5 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -1045,6 +1045,21 @@ class Int(Number):
         ('maxvalue', (int, long), int(MAXINT)),
     )
 
+    @staticmethod
+    def convert_int(value):
+        if type(value) in Int.allowed_types:
+            return value
+
+        if type(value) is float:
+            return int(value)
+
+        if type(value) is unicode:
+            if u'.' in value:
+                return int(float(value))
+            return int(value, 0)
+
+        raise ValueError(value)
+
     def __init__(self, name, *rules, **kw):
         super(Int, self).__init__(name, *rules, **kw)
 
@@ -1058,30 +1073,12 @@ def _convert_scalar(self, value, index=None):
         """
         Convert a single scalar value.
         """
-        if type(value) in (int, long):
-            return value
-        if type(value) is unicode:
-            # permit floating point strings
-            if value.find(u'.') >= 0:
-                try:
-                    return int(float(value))
-                except ValueError:
-                    pass
-            else:
-                try:
-                    # 2nd arg is radix base, 2nd arg only accepted for strings.
-                    # Zero means determine radix base from prefix (e.g. 0x for hex)
-                    return int(value, 0)
-                except ValueError:
-                    pass
-        if type(value) is float:
-            try:
-                return int(value)
-            except ValueError:
-                pass
-        raise ConversionError(name=self.get_param_name(), index=index,
-            error=ugettext(self.type_error),
-        )
+        try:
+            return Int.convert_int(value)
+        except ValueError:
+            raise ConversionError(name=self.get_param_name(),
+                                  index=index,
+                                  error=ugettext(self.type_error))
 
     def _rule_minvalue(self, _, value):
         """
@@ -1549,6 +1546,27 @@ class StrEnum(Enum):
     type = unicode
 
 
+class IntEnum(Enum):
+    """
+    Enumerable for integer data (stored in the ``int`` type).
+    """
+
+    type = int
+    types = int, long
+    type_error = Int.type_error
+
+    def _convert_scalar(self, value, index=None):
+        """
+        Convert a single scalar value.
+        """
+        try:
+            return Int.convert_int(value)
+        except ValueError:
+            raise ConversionError(name=self.get_param_name(),
+                                  index=index,
+                                  error=ugettext(self.type_error))
+
+
 class Any(Param):
     """
     A parameter capable of holding values of any type. For internal use only.
-- 
1.8.3.1

From 6753985180333b23553fa4f9964ef4ca4d75dbb4 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Mon, 7 Oct 2013 11:00:15 +0200
Subject: [PATCH] Add tests for the IntEnum class

The StrEnum and Int tests are restructured to allow sharing the tests.
Individual *Enum tests are separated into methods.
---
 ipatests/test_ipalib/test_parameters.py | 154 +++++++++++++++++++++-----------
 1 file changed, 100 insertions(+), 54 deletions(-)

diff --git a/ipatests/test_ipalib/test_parameters.py b/ipatests/test_ipalib/test_parameters.py
index 22c7b7355d60570aa03285562f0b9e6ddf4932d6..d1956beded9ad07a408db0e6fc230bb6c3497e43 100644
--- a/ipatests/test_ipalib/test_parameters.py
+++ b/ipatests/test_ipalib/test_parameters.py
@@ -1072,76 +1072,140 @@ def test_convert_scalar(self):
         assert o._convert_scalar(u'one') == u'one'
 
 
-class test_StrEnum(ClassChecker):
+class EnumChecker(ClassChecker):
     """
-    Test the `ipalib.parameters.StrEnum` class.
+    Test *Enum classes.
     """
     _cls = parameters.StrEnum
 
     def test_init(self):
-        """
-        Test the `ipalib.parameters.StrEnum.__init__` method.
-        """
-        values = (u'Hello', u'naughty', u'nurse!')
-        o = self.cls('my_strenum', values=values)
-        assert o.type is unicode
+        """Test the `__init__` method"""
+        values = self._test_values
+        o = self.cls(self._name, values=values)
+        assert o.type is self._datatype
         assert o.values is values
         assert o.class_rules == (o._rule_values,)
         assert o.rules == tuple()
         assert o.all_rules == (o._rule_values,)
 
-        badvalues = (u'Hello', 'naughty', u'nurse!')
+    def test_bad_types(self):
+        """Test failure with incorrect types"""
+        badvalues = self._bad_type_values
         e = raises(TypeError, self.cls, 'my_enum', values=badvalues)
         assert str(e) == TYPE_ERROR % (
-            "StrEnum('my_enum') values[1]", unicode, 'naughty', str
-        )
+            "%s('my_enum') values[1]" % self._cls.__name__,
+            self._datatype, badvalues[1], self._bad_type)
 
-        # Test that ValueError is raised when list of values is empty
+    def test_empty(self):
+        """Test that ValueError is raised when list of values is empty"""
         badvalues = tuple()
         e = raises(ValueError, self.cls, 'empty_enum', values=badvalues)
-        assert_equal(str(e), "StrEnum('empty_enum'): list of values must not "
-                "be empty")
+        assert_equal(str(e), "%s('empty_enum'): list of values must not "
+                     "be empty" % self._cls.__name__)
 
     def test_rules_values(self):
-        """
-        Test the `ipalib.parameters.StrEnum._rule_values` method.
-        """
-        values = (u'Hello', u'naughty', u'nurse!')
-        o = self.cls('my_enum', values=values)
-        rule = o._rule_values
-        translation = u"values='Hello', 'naughty', 'nurse!'"
-        dummy = dummy_ugettext(translation)
+        """Test the `_rule_values` method"""
 
-        # Test with passing values:
-        for v in values:
+    def test_rules_with_passing_rules(self):
+        """Test with passing values"""
+        o = self.cls('my_enum', values=self._test_values)
+        rule = o._rule_values
+        dummy = dummy_ugettext(self._translation)
+        for v in self._test_values:
             assert rule(dummy, v) is None
             assert dummy.called() is False
 
-        # Test with failing values:
-        for val in (u'Howdy', u'quiet', u'library!'):
+    def test_rules_with_failing_rules(self):
+        """Test with failing values"""
+        o = self.cls('my_enum', values=self._test_values)
+        rule = o._rule_values
+        dummy = dummy_ugettext(self._translation)
+        for val in self._bad_values:
             assert_equal(
                 rule(dummy, val),
-                translation % dict(values=values),
+                self._translation % dict(values=self._test_values),
             )
             assert_equal(dummy.message, "must be one of %(values)s")
             dummy.reset()
 
-        # test a special case when we have just one allowed value
-        values = (u'Hello', )
+    def test_one_value(self):
+        """test a special case when we have just one allowed value"""
+        values = (self._test_values[0], )
         o = self.cls('my_enum', values=values)
         rule = o._rule_values
-        translation = u"value='Hello'"
-        dummy = dummy_ugettext(translation)
+        dummy = dummy_ugettext(self._single_value_translation)
 
-        for val in (u'Howdy', u'quiet', u'library!'):
+        for val in self._bad_values:
             assert_equal(
                 rule(dummy, val),
-                translation % dict(values=values),
+                self._single_value_translation % dict(values=values),
             )
             assert_equal(dummy.message, "must be '%(value)s'")
             dummy.reset()
 
 
+class test_StrEnum(EnumChecker):
+    """
+    Test the `ipalib.parameters.StrEnum` class.
+    """
+    _cls = parameters.StrEnum
+    _name = 'my_strenum'
+    _datatype = unicode
+    _test_values = u'Hello', u'naughty', u'nurse!'
+    _bad_type_values = u'Hello', 'naughty', u'nurse!'
+    _bad_type = str
+    _translation = u"values='Hello', 'naughty', 'nurse!'"
+    _bad_values = u'Howdy', u'quiet', u'library!'
+    _single_value_translation = u"value='Hello'"
+
+
+def check_int_scalar_conversions(o):
+    """
+    Assure radix prefixes work, str objects fail,
+    floats (native & string) are truncated,
+    large magnitude values are promoted to long,
+    empty strings & invalid numerical representations fail
+    """
+    # Assure invalid inputs raise error
+    for bad in ['hello', u'hello', True, None, u'', u'.', 8j, ()]:
+        e = raises(errors.ConversionError, o._convert_scalar, bad)
+        assert e.name == 'my_number'
+        assert e.index is None
+    # Assure large magnitude values are handled correctly
+    assert type(o._convert_scalar(sys.maxint * 2)) == long
+    assert o._convert_scalar(sys.maxint * 2) == sys.maxint * 2
+    assert o._convert_scalar(unicode(sys.maxint * 2)) == sys.maxint * 2
+    assert o._convert_scalar(long(16)) == 16
+    # Assure normal conversions produce expected result
+    assert o._convert_scalar(u'16.99') == 16
+    assert o._convert_scalar(16.99) == 16
+    assert o._convert_scalar(u'16') == 16
+    assert o._convert_scalar(u'0x10') == 16
+    assert o._convert_scalar(u'020') == 16
+
+
+class test_IntEnum(EnumChecker):
+    """
+    Test the `ipalib.parameters.IntEnum` class.
+    """
+    _cls = parameters.IntEnum
+    _name = 'my_intenum'
+    _datatype = int
+    _test_values = 1, 2, -3
+    _bad_type_values = 1, 2.0, -3
+    _bad_type = float
+    _translation = u"values=1, 2, 3"
+    _bad_values = 4, 5, -6
+    _single_value_translation = u"value=1"
+
+    def test_convert_scalar(self):
+        """
+        Test the `ipalib.parameters.IntEnum._convert_scalar` method.
+        """
+        param = self.cls('my_number', values=(1, 2, 3, 4, 5))
+        check_int_scalar_conversions(param)
+
+
 class test_Number(ClassChecker):
     """
     Test the `ipalib.parameters.Number` class.
@@ -1239,28 +1303,10 @@ def test_rule_maxvalue(self):
     def test_convert_scalar(self):
         """
         Test the `ipalib.parameters.Int._convert_scalar` method.
-        Assure radix prefixes work, str objects fail,
-        floats (native & string) are truncated,
-        large magnitude values are promoted to long,
-        empty strings & invalid numerical representations fail
         """
-        o = self.cls('my_number')
-        # Assure invalid inputs raise error
-        for bad in ['hello', u'hello', True, None, u'', u'.']:
-            e = raises(errors.ConversionError, o._convert_scalar, bad)
-            assert e.name == 'my_number'
-            assert e.index is None
-        # Assure large magnatude values are handled correctly
-        assert type(o._convert_scalar(sys.maxint*2)) == long
-        assert o._convert_scalar(sys.maxint*2) == sys.maxint*2
-        assert o._convert_scalar(unicode(sys.maxint*2)) == sys.maxint*2
-        assert o._convert_scalar(long(16)) == 16
-        # Assure normal conversions produce expected result
-        assert o._convert_scalar(u'16.99') == 16
-        assert o._convert_scalar(16.99)    == 16
-        assert o._convert_scalar(u'16')    == 16
-        assert o._convert_scalar(u'0x10')  == 16
-        assert o._convert_scalar(u'020')   == 16
+        param = self.cls('my_number')
+        check_int_scalar_conversions(param)
+
 
 class test_Decimal(ClassChecker):
     """
-- 
1.8.3.1

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to