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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel