[Freeipa-devel] [PATCHES] 0761-0769 More Python3 fixes

[Jan Cholasta]

On 9.2.2016 18:02, Petr Viktorin wrote:
> On 01/29/2016 09:42 AM, Jan Cholasta wrote:
>> On 29.1.2016 09:25, Jan Cholasta wrote:
>>> Hi,
>>>
>>> On 27.1.2016 18:38, Petr Viktorin wrote:
>>>> Hello,
>>>>
>>>> Here is a mixed bag of Python 3 fixes.
>>>> They fix some tests, and they should enable you to use `python3
>>>> /usr/bin/ipa`.
>>>
>>> Patch 761:
>>>
>>> 1) The "invalid 'my_number': " bit comes from IPA itself, shouldn't we
>>> check at least that?
>
> Fixed
>
>>> Patch 762:
>>>
>>> 1) We should handle UnicodeError here as well, in addition to TypeError:
>>>
>>>                    if k.lower() == 'negotiate':
>>>                        try:
>>> -                        token = base64.b64decode(v)
>>> +                        token = base64.b64decode(v.encode('ascii'))
>>>                            break
>>>                        # b64decode raises TypeError on invalid input
>>>                        except TypeError:
>
> Fixed
>
>>> 2) I would prefer if the encoding was specified explicitly here:
>>>
>>> +            response = json_decode_binary(json.loads(response.decode()))
>
> Fixed
>
>>> Patch 763:
>>>
>>> 1)
>>>
>>> +                    altname = altname
>
> Fixed
>
>>> 2) Nitpick, but could you please:
>>>
>>> -        if isinstance(name_or_oid, unicode):
>>> -            name_or_oid = name_or_oid.encode('utf-8')
>>> +        if six.PY2:
>>> +            if isinstance(name_or_oid, unicode):
>>> +                name_or_oid = name_or_oid.encode('utf-8')
>>>
>>> This way it's more visible that this is a py2-only thing.
>
> Sure
>
>>> Patch 764: LGTM
>>>
>>>
>>> Patch 765:
>>>
>>> 1)
>>>
>>> +import tempfile
>>> +import tempfile
>
> Fixed.
>
>>> Patch 766-767: LGTM
>>>
>>>
>>> Patch 768:
>>>
>>> 1) Only binascii.Error should be handled in int_to_bytes, the try-except
>>> block is there just to handle odd-length strings.
>
> That's there for Python 2, where unhexlify raises TypeError.
>
>>> 2) I think you can just remove the library_path.encode(), it's there
>>> because the original C code did the same thing, but don't think it's
>>> necessary.
>
> OK
>
>>> Patch 769: LGTM
>>
>> Also, could you please reference
>> <https://fedorahosted.org/freeipa/ticket/5638> in the patches?
>
> Sure.
>
> Thanks for the review! Updated patches attached.

Thanks, ACK.

Pushed to:
master: d1252cfb8ef613e290a23e0e9cabd9d135a129ca
ipa-4-3: c8c2a6d33815b478c1d6ec402eaa2f56d0fe61b7

-- 
Jan Cholasta