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

[Petr Viktorin]

On 10/09/2013 04:17 PM, Nathaniel McCallum wrote:
> On Wed, 2013-10-09 at 10:23 +0200, Petr Viktorin wrote:
>> 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.
>
> Two smaller issues.
>
> You define allowed_types as a @property in Param and then as a tuple in
> Int. This seems stylistically inconsistent, though I understand why
> you've done this, it breaks a certain understanding about the behavior
> of subclasses of Int.

I don't think that's much of an issue. Using a @property that would 
always return the same value seems redundant.
What understanding did you have in mind?

> Also, in IntEnum, you've set IntEnum.types rather than
> IntEnum.allowed_types.

Fixed, thanks.

-- 
Petr³

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0001.3-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/40f7648f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0002.3-Add-IntEnum-parameter-to-ipalib.patch
Type: text/x-patch
Size: 3968 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131009/40f7648f/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0003.3-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/40f7648f/attachment-0002.bin>