<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">On 22.12.2015 13:43, Martin Basti wrote: </div> <blockquote cite="mid:5679458D.9090207@redhat.com" type="cite"> On 22.12.2015 08:06, Jan Cholasta wrote: <blockquote type="cite">On 17.12.2015 14:18, Martin Basti wrote: <blockquote type="cite"> On 17.12.2015 07:54, Jan Cholasta wrote: <blockquote type="cite">Hi, On 17.12.2015 02:11, Martin Basti wrote: <blockquote type="cite">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. </blockquote> Would it be possible to turn this into a blacklist of disabled warnings rather than a whitelist of enabled warnings? <blockquote type="cite"> 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 </blockquote> Honza </blockquote> Updated patches attached. </blockquote> 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 <a class="moz-txt-link-rfc2396E" href="http://docs.pylint.org/output.html"><http://docs.pylint.org/output.html></a>, 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.) </blockquote> 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 <fieldset class="mimeAttachmentHeader"></fieldset> </blockquote> I forgot to attach patch 379, all patches again (394 should go first) </body> </html>