[Freeipa-devel] [PATCH 0394, 0379 - 0393] Winter code cleanup

Tue Dec 22 14:18:47 UTC 2015


On 22.12.2015 13:43, Martin Basti wrote:
>
>
> On 22.12.2015 08:06, Jan Cholasta wrote:
>> On 17.12.2015 14:18, Martin Basti wrote:
>>>
>>>
>>> On 17.12.2015 07:54, Jan Cholasta wrote:
>>>> Hi,
>>>>
>>>> On 17.12.2015 02:11, Martin Basti wrote:
>>>>> Hello,
>>>>> following patches cleanup the code and add checks to pylint to 
>>>>> avoid the
>>>>> mess again
>>>>>
>>>>> Patches: 379-381:
>>>>> the most important patches, they cleanup imports
>>>>>
>>>>> Patch 382:
>>>>> enables various pylint checks, we may reduce the list of check if
>>>>> needed.
>>>>
>>>> Would it be possible to turn this into a blacklist of disabled
>>>> warnings rather than a whitelist of enabled warnings?
>>>>
>>>>>
>>>>> Patches 383 - 393:
>>>>> remove minor issues from code, and enable particular checks
>>>>> Feel free to nack patches/checks that should not be there.
>>>>>
>>>>> Please apply patches in order.
>>>>>
>>>>> Martin^2
>>>>>
>>>>>
>>>>
>>>> Honza
>>>>
>>> Updated patches attached.
>>
>> Patch 379: ACK
>>
>>
>> Patch 380:
>>
>> 1) This patch needs to be rebased.
>>
>>
>> 2) make-lint is reporting this to me:
>>
>> ************* Module ipatests.test_cmdline.test_ipagetkeytab
>> ipatests/test_cmdline/test_ipagetkeytab.py:30: [W0611(unused-import), 
>> ] Unused import nose)
>> ipatests/test_cmdline/test_ipagetkeytab.py:34: [W0611(unused-import), 
>> ] Unused DN imported from ipapython.dn)
>> ************* Module ipatests.test_cmdline.test_help
>> ipatests/test_cmdline/test_help.py:21: [W0611(unused-import), ] 
>> Unused import contextlib)
>> ipatests/test_cmdline/test_help.py:23: [W0611(unused-import), ] 
>> Unused assert_raises imported from nose.tools)
>> ************* Module ipatests.test_cmdline.test_cli
>> ipatests/test_cmdline/test_cli.py:11: [W0611(unused-import), ] Unused 
>> API_VERSION imported from ipapython.version)
>> ************* Module ipaplatform.services
>> ipaplatform/services.py:59: [W0611(unused-import), ] Unused 
>> timedate_services imported from ipaplatform.redhat.services)
>> ************* Module ipaplatform.rhel.services
>> ipaplatform/rhel/services.py:59: [W0611(unused-import), ] Unused 
>> timedate_services imported from ipaplatform.redhat.services)
>> ************* Module ipaplatform.redhat.services
>> ipaplatform/redhat/services.py:306: [W0611(unused-import), ] Unused 
>> timedate_services imported from ipaplatform.base.services)
>> ************* Module ipaplatform.fedora.services
>> ipaplatform/fedora/services.py:59: [W0611(unused-import), ] Unused 
>> timedate_services imported from ipaplatform.redhat.services)
>> ************* Module ipalib.setup
>> ipalib/setup.py:28: [W0611(unused-import), ] Unused import 
>> distutils.sysconfig)
>>
>> Note that the "from ipaplatform.${PLATFORM}.services import 
>> timedate_services" in services.py should be rewritten to 
>> "timedate_services = ${PLATFORM}_services.timedate_services" to fix 
>> this.
>>
>>
>> Patch 381: ACK
>>
>>
>> Patch 382:
>>
>> Nitpick: according to <http://docs.pylint.org/output.html>, the order 
>> of message categories is F, E, W, C, R from the most severe to the 
>> least severe (it does not mention I, but I think it should be last, 
>> after R), IMO we should keep this order here as well for clarity. 
>> (Don't worry, doing this should not require rebasing the following 
>> patches.)
>>
>>
>> Patch 383 - 387: ACK
>>
>>
>> Patch 388:
>>
>> IMO it would be better to rewrite this:
>>
>>             [(t.id, t.options) for t in doc.getKeyPackages()]  # 
>> pylint: disable=expression-not-assigned
>>
>> into this:
>>
>>     for t in doc.getKeyPackages():
>>         t._PSKCKeyPackage__process()
>>
>> rather than disabling the check.
>>
>>
>> Patch 389:
>>
>> All this patch does is that it enables a check globally and then 
>> disables it everywhere locally.
>>
>> IMO the patch should be retired for now, or make-lint should be 
>> taught to automatically skip the check inside TestCase.assertRaises() 
>> context.
>>
>>
>> Patch 390: ACK
>>
>>
>> Patch 391:
>>
>> default_from uses argument names of the specified function as param 
>> names from which to create the default.
>>
>> These changes break default_from:
>>
>> -            default_from=lambda name_from_ip: 
>> _reverse_zone_name(name_from_ip),
>> +            default_from=_reverse_zone_name,
>>
>> -            default_from=lambda idnsname: 
>> default_zone_update_policy(idnsname),
>> +            default_from=default_zone_update_policy,
>>
>> This change adds dependency on non-existent param:
>>
>> -            default_from=lambda: krb_utils.get_principal(),
>> +            default_from=krb_utils.get_principal,
>>
>> The result of this check is misleading for default_from, so IMO the 
>> patch should be retired for now, or make-lint should be taught to 
>> automatically disable the check for the default_from argument in 
>> param definitions.
>>
>>
>> Patch 392:
>>
>> I think the existing occurences of exec()/eval() should be removed 
>> before this check can be enabled.
>>
>>
>> Patch 393: ACK
>>
>>
>> I would like to see a patch which enables the useless-suppression 
>> info message, so that we can catch dangling "# pylint: disable=" 
>> comments (there are some in the code).
>>
>> (Also, it would be nice to finally rewrite make-lint to pylint config 
>> file + plugins.)
>>
> Updated patches attached, please apply patch 394 first.
>
> Patches 389, 391, 392 have been removed, issues will be addressed later.
>
> useless-suppression, unbalanced-tuple-unpacking, 
> unpacking-non-sequence, and python3 checks are on my TODO list
>
> I can try to rewrite pylint to plugins and config file.
>
> Martin^2
>
>
I forgot to attach patch 379, all patches again (394 should go first)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151222/2fa5023f/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0379.3-Remove-empty-test-file.patch
Type: text/x-patch
Size: 1537 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151222/2fa5023f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0380.3-Remove-unused-imports.patch
Type: text/x-patch
Size: 106936 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151222/2fa5023f/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0381.3-Remove-wildcard-imports.patch
Type: text/x-patch
Size: 45573 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151222/2fa5023f/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0382.3-Enable-multiple-warnings-checks-in-Pylint.patch
Type: text/x-patch
Size: 4726 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151222/2fa5023f/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0383.3-Enable-pylint-lost-exception-check.patch
Type: text/x-patch
Size: 1667 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151222/2fa5023f/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0384.3-Enable-pylint-duplicated-key-check.patch
Type: text/x-patch
Size: 1918 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151222/2fa5023f/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0385.3-Enable-pylint-trailing-whitespace-check.patch
Type: text/x-patch
Size: 3827 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151222/2fa5023f/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0386.3-Enable-pylint-missing-final-newline-check.patch
Type: text/x-patch
Size: 1637 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151222/2fa5023f/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0387.3-Enable-pylint-unused-format-string-key-check.patch
Type: text/x-patch
Size: 2774 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151222/2fa5023f/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0388.3-Enable-pylint-expression-not-assigned-check.patch
Type: text/x-patch
Size: 8068 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151222/2fa5023f/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0390.3-Enable-pylint-empty-docstring-check.patch
Type: text/x-patch
Size: 2590 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151222/2fa5023f/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0393.3-Enable-pylint-unnecessary-pass-check.patch
Type: text/x-patch
Size: 11020 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151222/2fa5023f/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0394-Use-module-variables-for-timedate_services.patch
Type: text/x-patch
Size: 2192 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151222/2fa5023f/attachment-0012.bin>