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

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