On (10/12/15 13:36), Petr Cech wrote:
>On 12/10/2015 01:27 PM, Lukas Slebodnik wrote:
>>On (10/12/15 12:54), Petr Cech wrote:
>>>On 11/23/2015 03:09 PM, Jakub Hrozek wrote:
>>>>On Wed, Nov 11, 2015 at 12:26:41PM +0100, Jakub Hrozek wrote:
>>>>>When Python3 was used, the tests used our own compat function for
>>>>>checking sequences. It's better to stick to the standard-library
>>>>>provided methods.
>>>>>
>>>>>More details are in the commit message..
>>>>
>>>>Sorry, the patch wasn't correct, a new one is attached.
>>>>
>>>
>>>Hi Jakub,
>>>
>>>I have done review. Code looks good to me and I catched this:
>>>
>>>http://sssd-ci.duckdns.org/logs/job/34/36/summary.html
>>>
>>>I asked the CI list... ok, it is not connected to your patch, so:
>>>
>>CI passed but new pep8 warnings were introduced.
>>We get rid of them in integration best.
>>But we needn't introduce new in other python files.
>>
>>sh$ git diff HEAD~..HEAD | pep8 --diff
>>./src/tests/pyhbac-test.py:17:1: E302 expected 2 blank lines, found 1
>>./src/tests/pyhbac-test.py:20:1: E302 expected 2 blank lines, found 1
>>./src/tests/pyhbac-test.py:39:1: E302 expected 2 blank lines, found 1
>>./src/tests/pyhbac-test.py:75:1: E302 expected 2 blank lines, found 1
>>./src/tests/pyhbac-test.py:83:18: E201 whitespace after '['
>>./src/tests/pyhbac-test.py:83:31: E202 whitespace before ']'
>>./src/tests/pyhbac-test.py:87:19: E201 whitespace after '['
>>./src/tests/pyhbac-test.py:87:32: E202 whitespace before ']'
>>./src/tests/pyhbac-test.py:92:18: E201 whitespace after '['
>>./src/tests/pyhbac-test.py:92:31: E202 whitespace before ']'
>>./src/tests/pyhbac-test.py:98:19: E201 whitespace after '['
>>./src/tests/pyhbac-test.py:98:32: E202 whitespace before ']'
>>./src/tests/pyhbac-test.py:105:19: E201 whitespace after '('
>>./src/tests/pyhbac-test.py:105:32: E202 whitespace before ')'
>>./src/tests/pyhbac-test.py:138:42: E201 whitespace after '['
>>./src/tests/pyhbac-test.py:138:55: E202 whitespace before ']'
>>./src/tests/pyhbac-test.py:139:43: E201 whitespace after '['
>>./src/tests/pyhbac-test.py:139:56: E202 whitespace before ']'
>>./src/tests/pyhbac-test.py:226:80: E501 line too long (90 > 79 characters)
>>./src/tests/pyhbac-test.py:236:50: E201 whitespace after '['
>>./src/tests/pyhbac-test.py:236:63: E202 whitespace before ']'
>>./src/tests/pyhbac-test.py:237:51: E201 whitespace after '['
>>./src/tests/pyhbac-test.py:237:64: E202 whitespace before ']'
>>./src/tests/pyhbac-test.py:269:41: E201 whitespace after '('
>>./src/tests/pyhbac-test.py:272:78: E202 whitespace before ')'
>>./src/tests/pyhbac-test.py:272:80: E501 line too long (80 > 79 characters)
>>./src/tests/pyhbac-test.py:279:41: E201 whitespace after '('
>>./src/tests/pyhbac-test.py:280:78: E202 whitespace before ')'
>>./src/tests/pyhbac-test.py:280:80: E501 line too long (80 > 79 characters)
>>./src/tests/pyhbac-test.py:282:29: E201 whitespace after '['
>>./src/tests/pyhbac-test.py:282:37: E202 whitespace before ']'
>>./src/tests/pyhbac-test.py:288:1: E302 expected 2 blank lines, found 1
>>./src/tests/pyhbac-test.py:299:19: E201 whitespace after '['
>>./src/tests/pyhbac-test.py:299:32: E202 whitespace before ']'
>>./src/tests/pyhbac-test.py:310:19: E201 whitespace after '['
>>./src/tests/pyhbac-test.py:310:32: E202 whitespace before ']'
>>./src/tests/pyhbac-test.py:317:19: E201 whitespace after '('
>>./src/tests/pyhbac-test.py:317:32: E202 whitespace before ')'
>>./src/tests/pysss_murmur-test.py:29:28: E261 at least two spaces before inline
comment
>>./src/tests/pysss_murmur-test.py:29:29: E262 inline comment should start with
'# '
>>./src/tests/pysss_murmur-test.py:31:1: E302 expected 2 blank lines, found 1
>>
>>LS
>
>Well, that seems to be valid point. I take back my ACK.
>Lukas, I see that you run PEP8 locally. Is there plan to add PEP8 to CI?
>
we have many pep8 warnings in old python code
https://fedorahosted.org/sssd/ticket/2774
that's the reason why I shared command how to checked newly introduced
warnings.
git diff HEAD~..HEAD | pep8 --diff
Feel free to integrate this command to CI.
I fixed the new pep8 warnings in the first patch and all the others in
that module in the second patch.
Thanks for the review.