[Freeipa-devel] pylint: remove unused variables

[Tomas Krizek]

On 09/22/2016 06:00 PM, Martin Basti wrote:
>
>
> On 22.09.2016 17:59, Tomas Krizek wrote:
>> On 09/22/2016 04:39 PM, Martin Basti wrote:
>>> Hello all,
>>>
>>> In 4.5, I would like to remove all unused variables from code and 
>>> enable pylint check. Due to big amount of unused variables in the 
>>> code this will be longterm effort.
>>>
>>> Why this?:
>>>
>>> * better code readability
>>>
>>> * removing dead code
>>>
>>> * unused variable may uncover potential bug
>>>
>>>
>>> It is clear what to do with unused assignments, but I need an 
>>> agreement what to do with unpacking or iteration with unused variables
>>>
>>>
>>> For example:
>>>
>>> for name, surname, gender in (('Martin', 'Basti', 'M'), ):
>>>
>>> name, surname, gender = user['mbasti']
>>>
>>> Where 'surname' is unused
>>>
>>>
>>> Pylint will detect surname as unused variable and we have to agree 
>>> on a way how to tell pylint that this variable is unused on purpose:
>>>
>>>
>>> 1)
>>>
>>> (
>>>
>>>    name,
>>>
>>>    surname,  # pylint: disable=unused-variable
>>>
>>>    gender
>>>
>>> ) = user['mbasti']
>>>
>>>
>>> I dont like this approach
>>>
>>>
>>> 2)
>>>
>>> Use defined keyword: 'dummy' is default in pylint, we can set our 
>>> own, like ignored, unused
>>>
>>> name, dummy, gender = user['mbasti']
>>>
>>>
>>> 3)
>>>
>>> use a prefix for unused variables: '_' or 'ignore_'
>>>
>>> name, _surname, gender = user['mbasti']
>>>
>>>
>>> 4)
>>>
>>> we can combine all :)
>>>
>>>
>>> For me the best is to have prefix '_' and 'dummy' keyword
>>>
>>>
>>> As first step I'll enable pylint check and disable it locally per 
>>> module where unused variables are, to avoid new regressions. Then I 
>>> will fix it module by module.
>>>
>>>
>>> I'm open to suggestions
>>>
>>> Martin^2
>>>
>> I'd use a double underscore variable:
>>
>> name, __, gender = user['mbasti']
>>
>> It is quicker to write than _dummy and it also provides a better 
>> readability, because I can immediately identify the symbol as 
>> special. Unlike _dummy which I have to read to understand (simply 
>> because I'm used to _something to indicate a 'protected' variable).
>>
>
> With double underscores there is a risk, that typo will be just one 
> underscore and _ means ugettext in IPA :)
I think pylint would detect a redefinition in that case, since this 
check was added in today's PR#97:

https://github.com/freeipa/freeipa/pull/97/commits/06f35e5bdcb9f3ea42145de253674fda06b43d30

-- 
Tomas Krizek