On Thu, Dec 10, 2015 at 01:44:26PM +0100, Lukas Slebodnik wrote:
> 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.
From 89e56266a17661d6219107912b4501fbb1071bf2 Mon Sep 17 00:00:00
2001
From: Sumit Bose <sbose(a)redhat.com>
Date: Mon, 21 Dec 2015 15:51:09 +0100
Subject: [PATCH] ldap: remove originalMeberOf if there is no memberOf