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

[Petr Viktorin]

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³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001.2-Allow-multiple-types-in-Param-type-validation.patch
Type: text/x-patch
Size: 6830 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131009/91ae9b44/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002.2-Add-IntEnum-parameter-to-ipalib.patch
Type: text/x-patch
Size: 3960 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131009/91ae9b44/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003.2-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/20131009/91ae9b44/attachment-0002.bin>