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

[Nathaniel McCallum]

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.

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

Nathaniel