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

[Petr Viktorin]

On 10/09/2013 05:44 PM, Nathaniel McCallum wrote:
> On Wed, 2013-10-09 at 16:36 +0200, Petr Viktorin wrote:
>> 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?
>
> I don't have a better option. It just stood out to me.
>
>>> Also, in IntEnum, you've set IntEnum.types rather than
>>> IntEnum.allowed_types.
>>
>> Fixed, thanks.
>
> I think this is ready to merge. Do you?

Thanks! Let's get it in.
Pushed to master: 5e8aab8558874a9a826a1c470e806c75fb84eef2

-- 
Petr³