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

[Jan Cholasta]

On 22.12.2015 15:18, Martin Basti wrote:
>
>
> 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.

OK.

>>
>> 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.

Good!

>>
>> Martin^2
>>
>>
> I forgot to attach patch 379, all patches again (394 should go first)

Thanks, ACK.

Pushed to master: 00fd28e02689d49f89429b663b16ce4ca484e7d6

-- 
Jan Cholasta