[Freeipa-devel] [PATCHES] 0696-0710 More modernization

Mon Aug 24 15:31:08 UTC 2015

On 08/24/2015 08:48 AM, Jan Cholasta wrote:
> On 24.8.2015 07:50, Jan Cholasta wrote:
>> On 21.8.2015 14:50, Christian Heimes wrote:
>>> On 2015-08-21 12:55, Petr Viktorin wrote:
>>>> On 08/14/2015 07:44 PM, Petr Viktorin wrote:
>>>>> Hello,
>>>>> These patches bring IPA another step towards compatibility with
>>>>> Python 3.
>>>>>
>>>>> Most of these were made by fixers from the "python-modernize" tool,
>>>>> but
>>>>> I reviewed and edited the results.
>>>>
>>>> Here are the patches rebased to current master.
>>>
>>> 0696.2-Remove-use-of-sys.exc_value
>>> ACK
>>>
>>>
>>> 0697.2-Don-t-use-a-tuple-in-function-arguments
>>> I prefer operator.itemgetter() over the hard-to-read lambda expression
>>> key=lambda k_v: (k_v[1], k_v[0]).
>>>>>> import operator
>>>>>> example = dict(a=3, ba=2, b=2, c=1)
>>>>>> sorted(example.items(), key=operator.itemgetter(1, 0))
>>> [('c', 1), ('b', 2), ('ba', 2), ('a', 3)]

Sure.

>>>
>>>
>>> 0698.2-Add-python-six-to-dependencies
>>> ACK
>>>
>>>
>>> 0699.2-Remove-the-unused-pygettext-script
>>> ACK
>>>
>>>
>>> 0700.2-Use-six.string_types-instead-of-basestring
>>> LGTM, but I need to have a closer look at some places.
>>> I noticed a couple of asserts that should be "if ... raise ValueError"
>>> instead. python -o disables asserts.
> 
> It seems you missed a few "basestring"s in ipapython/dn.py.

Oops. Added.

>>> 0701.2-Use-Python3-compatible-dict-method-names
>>> NACK
>>> Why are you replacing iteritems() with items() instead of using
>>> six.iteritems()?

It looks cleaner, and it will be easier to clean up after six is dropped.
Also, the performance difference is negligible if the whole thing is
iterated over. (On small dicts, which are the majority of what iteritems
was used on, items() is actually a bit faster on my machine.)

>>> Please use sorted(reference) instead of sorted(reference.keys()),
>>> set(tree) instead of set(tree.keys()) and list(somedict) instead of
>>> list(somedict.keys()), too. The keys() call is unnecessary and frowned
>>> upon.

I wouldn't always frown upon it; sometimes making it clear that you're
operating on a dict makes the code more readable.

But, I've made the changes.

>>> 0702.2-Replace-filter-calls-with-list-comprehensions
>>> In Python 2 list comprehensions leak the internal loop variable. It
>>> might be better to write a generator expression with list() instead of
>>> [] list comprehension.

Well, that just is (and always has been) something to watch out for in
Python 2. I don't think it's a good reason for more verbose syntax.

>>> 0703.2-Use-six.moves.input-instead-of-raw_input
>>> ACK
>>> The code is fine, but pylint won't like it. For Dogtag I had to disable
>>> pylint warnings W0622 and F0401.

Hm. For some reason, make-lint works fine on this code (f21, pylint
1.3.1). It fails on gssapi, but in code these patches don't touch.

It may be a problem with my setup, let me know if it is.

>>> 0704.2-Use-six.integer_types-instead-of-long-int
>>> ACK
>>> hint: For type checks you can also use the numbers module.
> 
> There are still some "(int, long)"s in ipalib/parameters.py,
> ipalib/rpc.py, ipalib/util.py, ipapython/cookie.py, ipapython/dn.py and
> ipapython/ipaldap.py

Changed, thanks.

> Also, there are bare "long"s in ipapython/install/cli.py,
> ipaserver/dcerpc.py, ipaserver/install/ipa_otptoken_import.py and
> ipatests/test_ipalib/test_parameters.py.

Right, but that's for another patch.

>>> 0705.2-Replace-uses-of-map
>>> See comment for 0702
> 
> It seems you missed a few "map()"s in ipalib/plugins/certprofile.py,
> ipalib/plugins/dns.py, ipalib/plugins/sudorule.py and
> ipatests/test_xmlrpc/test_add_remove_cert_cmd.py.

Thanks, added.

>>> 706.2-Use-next-function-on-iterators
>>> ACK
>>
>> These are generator objects in ipapython/install/core.py. I'm not sure
>> what the usual convention is,

That depends on whether you want to "get the next item" or "send None to
the generator". I think in this case it's the former.

>> but I would think that the gen.next()
>> calls should be replaced with gen.send(None) instead of next(gen), so
>> that the generators are accessed consistently using methods
>> (gen.send()/gen.throw()/gen.close()).

I don't see any send() calls except one used to implement "yield from"
itself. How often do you see send/throw/close used?

>>> 0707.2-Use-the-print-function
>>> LGTM
>>> There are too many chances to review. Let's hope the automatic
>>> conversion tool did its job correctly.
> 
> I see some print statements in ipapython/dn.py.

Right, changed

>>> 0708.2-Use-new-style-raise-syntax
>>> ACK
>>>
>>>
>>> 0709.2-Use-six.reraise
>>> ACK
>>
>> Instead of calling six.reraise from raise_exc_info, could you replace
>> the two occurences of raise_exc_info(exc_info) with
>> six.reraise(*exc_info) and remove raise_exc_info?

Right.

>>> 0710.2-Modernize-use-of-range
>>> NACK
>>> Please use six.moves.range. It defaults to xrange() in Python 2.

Why? What is the benefit of xrange in these situations?

Like with iteritems in 0701, when iterating over the whole thing, the
performance difference is negligible. I don't think a few microseconds
outside of tight loops are worth the verbosity.

>>> I also
>>> see a couple of additional opportunities for enumerate():
>>>
>>> for i in range(len(kw['attrs'])):
>>>      kw['attrs'][i] = unicode(kw['attrs'][i])
>>>
>>> for i, s in enumerate(kw['attrs']):
>>>      kw['attrs'][i] = unicode(s)

That one can be cleaned up a lot more:
-        kw['attrs'] = list(a.target['targetattr']['expression'])
-        for i in range(len(kw['attrs'])):
-            kw['attrs'][i] = unicode(kw['attrs'][i])
-        kw['attrs'] = tuple(kw['attrs'])
+        kw['attrs'] = [unicode(a)
+                       for a in a.target['targetattr']['expression']]

It wasn't the point of this patch, but I've made the change.

>>> 0711.2-Convert-zip-result-to-list
>>> ACK
>>> The code isn't beautiful but it's just a test.

-- 
Petr Viktorin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0696.3-Remove-use-of-sys.exc_value.patch
Type: text/x-patch
Size: 2529 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150824/6dcc45b4/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0697.3-Don-t-use-a-tuple-in-function-arguments.patch
Type: text/x-patch
Size: 907 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150824/6dcc45b4/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0698.3-Add-python-six-to-dependencies.patch
Type: text/x-patch
Size: 984 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150824/6dcc45b4/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0699.3-Remove-the-unused-pygettext-script.patch
Type: text/x-patch
Size: 30062 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150824/6dcc45b4/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0700.3-Use-six.string_types-instead-of-basestring.patch
Type: text/x-patch
Size: 35447 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150824/6dcc45b4/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0701.3-Use-Python3-compatible-dict-method-names.patch
Type: text/x-patch
Size: 73506 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150824/6dcc45b4/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0702.3-Replace-filter-calls-with-list-comprehensions.patch
Type: text/x-patch
Size: 6514 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150824/6dcc45b4/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0703.3-Use-six.moves.input-instead-of-raw_input.patch
Type: text/x-patch
Size: 3100 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150824/6dcc45b4/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0704.3-Use-six.integer_types-instead-of-long-int.patch
Type: text/x-patch
Size: 8301 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150824/6dcc45b4/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0705.3-Replace-uses-of-map.patch
Type: text/x-patch
Size: 18739 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150824/6dcc45b4/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0706.3-Use-next-function-on-iterators.patch
Type: text/x-patch
Size: 3481 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150824/6dcc45b4/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0707.3-Use-the-print-function.patch
Type: text/x-patch
Size: 229327 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150824/6dcc45b4/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0708.3-Use-new-style-raise-syntax.patch
Type: text/x-patch
Size: 6956 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150824/6dcc45b4/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0709.3-Use-six.reraise.patch
Type: text/x-patch
Size: 1892 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150824/6dcc45b4/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0710.3-Modernize-use-of-range.patch
Type: text/x-patch
Size: 18006 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150824/6dcc45b4/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0711.3-Convert-zip-result-to-list.patch
Type: text/x-patch
Size: 1223 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150824/6dcc45b4/attachment-0015.bin>