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

I just tested the patches and functionaly-wise it seems okay.

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to