[Freeipa-devel] [PATCH] pylint fixes

[Martin Basti]

On 01.07.2016 15:51, Florence Blanc-Renaud wrote:
> On 06/21/2016 01:51 PM, Martin Basti wrote:
>>
>>
>> On 21.06.2016 08:38, Florence Blanc-Renaud wrote:
>>> On 06/20/2016 07:08 PM, Martin Basti wrote:
>>>>
>>>>
>>>> On 20.06.2016 19:06, Martin Basti wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 20.06.2016 12:00, Florence Blanc-Renaud wrote:
>>>>>> On 06/09/2016 05:10 PM, Petr Spacek wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> I've received a bunch of pylint fixes produced by upstream
>>>>>>> contributor who is
>>>>>>> not subscribed to the list so I'm resending them here.
>>>>>>>
>>>>>>> All credit goes to Bárta Jan <55042barta at sstebrno.eu>.
>>>>>>>
>>>>>>> Flo, if you have time for it I think that it could be a good
>>>>>>> exercise which
>>>>>>> will lead you to various dark corners in IPA :-)
>>>>>>>
>>>>>>> Petr^2 Spacek
>>>>>>>
>>>>>>>
>>>>>>> -------- Forwarded Message --------
>>>>>>> Date: Fri, 3 Jun 2016 14:57:16 +0200
>>>>>>> From: Bárta Jan <55042barta at sstebrno.eu>
>>>>>>> To: pspacek at redhat.com
>>>>>> ___- In the patch
>>>>>> 0002-pylint-fix-simplifiable-if-statement-warnings.patch:_
>>>>>>
>>>>>> diff --git a/ipatests/test_integration/tasks.py
>>>>>> b/ipatests/test_integration/tasks.py
>>>>>> index aebd907..ca2e10f 100644
>>>>>> --- a/ipatests/test_integration/tasks.py
>>>>>> +++ b/ipatests/test_integration/tasks.py
>>>>>> @@ -149,11 +149,7 @@ def host_service_active(host, service):
>>>>>>      res = host.run_command(['systemctl', 'is-active', '--quiet',
>>>>>> service],
>>>>>>                             raiseonerr=False)
>>>>>>
>>>>>> -    if res.returncode == 0:
>>>>>> -        return True
>>>>>> -    else:
>>>>>> -        return False
>>>>>> -
>>>>>> +    return res.returncode
>>>>>>
>>>>>> should be instead: return res.returncode *== 0* (otherwise the 
>>>>>> return
>>>>>> type is an int and not a boolean).
>>>>>>
>>>>>> In the same file:
>>>>>> @@ -295,11 +291,7 @@ def
>>>>>> master_authoritative_for_client_domain(master, client):
>>>>>>      zone = ".".join(client.hostname.split('.')[1:])
>>>>>>      result = master.run_command(["ipa", "dnszone-show", zone],
>>>>>>                                  raiseonerr=False)
>>>>>> -    if result.returncode == 0:
>>>>>> -        return True
>>>>>> -    else:
>>>>>> -        return False
>>>>>> -
>>>>>> +    result.returncode == 0
>>>>>>
>>>>>> should be instead: *return* result.returncode == 0 (otherwise there
>>>>>> is no return statement)
>>>>>>
>>>>>> diff --git a/ipaserver/plugins/dogtag.py 
>>>>>> b/ipaserver/plugins/dogtag.py
>>>>>> index 197814c..36b6ba5 100644
>>>>>> --- a/ipaserver/plugins/dogtag.py
>>>>>> +++ b/ipaserver/plugins/dogtag.py
>>>>>> @@ -1689,12 +1689,7 @@ class ra(rabase.rabase):
>>>>>>          # Return command result
>>>>>>          cmd_result = {}
>>>>>>
>>>>>> -        if parse_result.get('revoked') == 'yes':
>>>>>> -            cmd_result['revoked'] = True
>>>>>> -        else:
>>>>>> -            cmd_result['revoked'] = False
>>>>>> -
>>>>>> -        return cmd_result
>>>>>> +        cmd_result['revoked'] = parse_result.get('revoked')
>>>>>>
>>>>>> Should be instead: cmd_result['revoked'] =
>>>>>> parse_result.get('revoked') *== 'yes'* (otherwise the type is a
>>>>>> string and not a boolean)
>>>>>>
>>>>>> _- in the patch 00__04-pylint-fix-unneeded-not.patch_
>>>>>>
>>>>>> @@ -632,7 +632,7 @@ class host_add(LDAPCreate):
>>>>>>                      options['ip_address'],
>>>>>>                      check_forward=True,
>>>>>>                      check_reverse=check_reverse)
>>>>>> -        if not options.get('force', False) and not 'ip_address' in
>>>>>> options:
>>>>>> +        if options.get('force', False) and 'ip_address' not in
>>>>>> options:
>>>>>>
>>>>>> Should be instead: if *not* options.get('force', False) and
>>>>>> 'ip_address' not in options:
>>>>>> because of operators precedence
>>>>>>
>>>>>> I will review patches 0005 to 0010 later today.
>>>>>> Flo.
>>>>>>
>>>>>>
>>>>>
>>>>> How about patches 1, and 3? Because patches are independent, we can
>>>>> separately ACK them and push them.
>>>>>
>>>>> Martin^2
>>>>>
>>>>>
>>>>
>>>> Sorry, I just noticed that there is no patch 1 :)
>>>>
>>>>
>>> Patch 0003 is OK, ACK for this one.
>>> Flo.
>>>
>> Patch 0003: Pushed to master: 94909d21dbf033cbe34089782c430ec25b9ad0bc
>>
> Hi,
>
> please find my comments on the remaining patches.
>
> - Patch 0005 must be rebased because of changes in 
> ipatests/test_integration/tasks.py
> the patch can also modify pylintrc (remove pointless-statement)
>
> - Patch 0006:
> no need to rename the items in "for e in ...", renaming the Exception 
> as exc should be enough
>
> - Patch 0007:
> pylintrc should remove old-style-class instead of 
> bad-classmethod-argument
>
> - Patch 0008:
> this one should remove bad-classmethod-argument in pylintrc
>
> - Patch 0009:
> ok
>
> - Patch 0010:
> In the __bind method(self, obj_type), cls variable is already used 
> thus replacing self with cls can be done only if cls is also renamed 
> into something else.
>
> Flo.
>

Please follow new changes in this PR 
https://github.com/freeipa/freeipa/pull/97
It will be easier to review.

Martin^2