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:

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

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

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

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

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:
+            default_from=_reverse_zone_name,

-            default_from=lambda 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.



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

Thanks, ACK.

Pushed to master: 00fd28e02689d49f89429b663b16ce4ca484e7d6

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