[Freeipa-devel] [PATCH 0407] make lint: migration to config file and pylint plugin due pylint 1.5.2

[David Kupka]

On 11/02/16 18:47, Martin Basti wrote:
>
>
> On 11.02.2016 11:08, Martin Basti wrote:
>>
>>
>> On 09.02.2016 15:26, David Kupka wrote:
>>> On 09/02/16 11:28, Martin Basti wrote:
>>>>
>>>>
>>>> On 03.02.2016 10:19, David Kupka wrote:
>>>>> On 01/02/16 14:18, Martin Basti wrote:
>>>>>>
>>>>>>
>>>>>> On 26.01.2016 14:16, Martin Basti wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 20.01.2016 14:38, Jan Cholasta wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 19.1.2016 13:43, Martin Basti wrote:
>>>>>>>>> New pylint version will broke our custom make-lint script again,
>>>>>>>>> attached patch migrates make-lint to:
>>>>>>>>> * config file
>>>>>>>>> * pylint plugin
>>>>>>>>> which are supported by pylint and should not have regular
>>>>>>>>> compatibility
>>>>>>>>> issues
>>>>>>>>>
>>>>>>>>> to test new approach run ./make-lint2
>>>>>>>>>
>>>>>>>>> Advantages:
>>>>>>>>> * compatibility with pylint
>>>>>>>>> * works on both pylint-1.4.3-3.fc23.noarch and
>>>>>>>>> pylint-1.5.2-1.fc24.noarch
>>>>>>>>> * pylint plugin works in different way than the previous custom
>>>>>>>>> checker.
>>>>>>>>> Missing ("dynamic") attributes are added to abstract syntax tree
>>>>>>>>> instead
>>>>>>>>> of ignoring them and all their sub-members. This makes check
>>>>>>>>> better,
>>>>>>>>> pylint can detect more typos in tests configurations, api, env,
>>>>>>>>> etc..
>>>>>>>>>
>>>>>>>>> Disadvantages:
>>>>>>>>> * any new attribute in api, test config, etc.. must be added to
>>>>>>>>> definition of missing members (pylint plugin) - this should not
>>>>>>>>> happen
>>>>>>>>> too often
>>>>>>>>
>>>>>>>> 1) Please "mv pylint_plugins/fix_ipa_members.py pylint_plugins.py"
>>>>>>>> and "rm -rf pylint_plugins/", no need for this redundant directory
>>>>>>>> structure.
>>>>>>>>
>>>>>>>> 2) Rename pylintrc to freeipa.pylintrc so you have to always
>>>>>>>> specify
>>>>>>>> it explicitly with --rcfile.
>>>>>>>>
>>>>>>>> 3) Use the load-plugins directive in freeipa.pylintrc to load the
>>>>>>>> plugins rather than --load-plugins.
>>>>>>>>
>>>>>>>> 4) Instead of running pylint twice, run it only once with both
>>>>>>>> normal
>>>>>>>> and Python 3 checks enabled:
>>>>>>>>
>>>>>>>>     [MESSAGE CONTROL]
>>>>>>>>     enable=all,python3
>>>>>>>>     disable=...,no-absolute-import
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Q&TODO:
>>>>>>>>> * make-lint: should it be just bash script or rather python
>>>>>>>>> script?
>>>>>>>>
>>>>>>>> IMO neither, it should be a make target (make lint).
>>>>>>>>
>>>>>>>>> * add dynamic detection of python files to be checked
>>>>>>>>
>>>>>>>> You can use "find . -type f -executable ! -path \*/.\* ! -name
>>>>>>>> \*.py\* -exec grep -lsm1 '^#!.*\bpython' \{\} \;".
>>>>>>>>
>>>>>>>>> * should I keep the current options from original make-lint?
>>>>>>>>
>>>>>>>> No, but allow pylint options to be overridable (make lint
>>>>>>>> PYLINTFLAGS="--disable=python3")
>>>>>>>>
>>>>>>>>> * several false positive errors I haven't been able to fix in
>>>>>>>>> plugin
>>>>>>>>> yet, in worst case they can be locally disabled:
>>>>>>>>
>>>>>>>> Disable them locally.
>>>>>>>>
>>>>>>>> Honza
>>>>>>>>
>>>>>>> Updated patch attached.
>>>>>>>
>>>>>>> Please note that make-lint script has been removed, to execute lint
>>>>>>> check use 'make lint'
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Updated patch attached:
>>>>>> * fixes recently added error
>>>>>> * fixes PEP8
>>>>>> * cleanup of pylint config file
>>>>>>
>>>>>> Patch is only for master branch, for 4.3 and 4.2 I will send
>>>>>> different
>>>>>> patches when this will be acked
>>>>>>
>>>>>>
>>>>>
>>>>> Could you please add an extensive comment to the 4-lines-long find
>>>>> command? I can after a while (and with the help of man page) decode
>>>>> what it does so it would definitely help to have it described.
>>>>> Otherwise it looks good to me.
>>>>>
>>>> Updated patch attached, only for master, patches for 4.2, 4.3 will
>>>> follow if this one will be ACKed
>>>
>>> The comment is probably sufficient, thanks.
>>> Also works for me on current master, ACK.
>>>
>> Pushed to master: 2ce8921fe69ed58871f8e33e3899ad80dcc28c0e
>>
>> Patches for 4.2 and 4.3 will follow
>>
> Patches for 4.2, 4.3 attached
>
>
Works for me, ACK.

-- 
David Kupka