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