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

[Nathaniel McCallum]

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?

Nathaniel