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

[Jan Cholasta]

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)]
>>
>>
>> 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.

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

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.

>>
>>
>> 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.

>>
>>
>> 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, 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()).
>
>>
>>
>> 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.

>>
>>
>> 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?
>
>>
>>
>> 0710.2-Modernize-use-of-range
>> NACK
>> Please use six.moves.range. It defaults to xrange() in Python 2. 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)
>>
>>
>> 0711.2-Convert-zip-result-to-list
>> ACK
>> The code isn't beautiful but it's just a test.
>>
>>
>>
>>
>
>


-- 
Jan Cholasta