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

[Jan Cholasta]

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.
>
>
> 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.
>
>
> 0705.2-Replace-uses-of-map
> See comment for 0702
>
>
> 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.
>
>
> 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