[Freeipa-devel] [PATCH] pylint fixes

[Florence Blanc-Renaud]

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.