On 17.12.2015 14:18, Martin Basti wrote:

On 17.12.2015 07:54, Jan Cholasta wrote:

On 17.12.2015 02:11, Martin Basti wrote:
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

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.



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():

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

Jan Cholasta

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

Reply via email to