[Freeipa-devel] [PATCH 0020] Add IntEnum parameter to ipalib

[Petr Viktorin]

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?

-- 
Petr³

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Allow-multiple-types-in-Param-type-validation.patch
Type: text/x-patch
Size: 8763 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131008/1f8fead2/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-IntEnum-parameter-to-ipalib.patch
Type: text/x-patch
Size: 2004 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131008/1f8fead2/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Add-tests-for-the-IntEnum-class.patch
Type: text/x-patch
Size: 8738 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131008/1f8fead2/attachment-0002.bin>