Feature #1137

Generally clean up existing code using pylint

Added by Rob Baer almost 2 years ago. Updated almost 2 years ago.

Status:NewStart date:02/25/2017
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:Code correctness
Target version:MakeHuman 1.1.2

Description

Feature Request: As we work on the python 3 transition, take opportunities to run pylint and clean the code appropriately to make it more robust. It's probably too late for 1.1.1, but it is eye-opening to run pylint on makehuman.py We need to figure ou how to make this a standard development practice.

Background and relation to #1136
Some time back Joel added pylint scans to Jenkins, and I've looked at them and thought he was right about the low hanging fruit this feedback presents. The transition to python 3 is an optimatal time to reconsider this.

With the early crash of #1136, it occurred to me to manually take a look at stable makehuman.py with pylint. As an example, advice about exceptio vulnerabilities might be useful. Here is what I found:

(python27) E:\makehuman-hg\makehuman>pylint makehuman.py
************* Module makehuman
makehuman.py:307: [W0511(fixme), ] TODO Disabled this fallback for now, it's possible to do this using the hg keyword extension, but not recommended and this metric was never really reliable (it only caused more confusion)
makehuman.py:697: [W0511(fixme), ] TODO print contributors.txt
makehuman.py:17: [C0303(trailing-whitespace), ] Trailing whitespace
makehuman.py:464: [C0303(trailing-whitespace), ] Trailing whitespace
makehuman.py:667: [C0303(trailing-whitespace), ] Trailing whitespace
makehuman.py:702: [C0303(trailing-whitespace), ] Trailing whitespace
makehuman.py:750: [C0303(trailing-whitespace), ] Trailing whitespace
makehuman.py:786: [C0303(trailing-whitespace), ] Trailing whitespace
makehuman.py:787: [C0303(trailing-whitespace), ] Trailing whitespace
makehuman.py:813: [C0303(trailing-whitespace), ] Trailing whitespace
makehuman.py:814: [C0303(trailing-whitespace), ] Trailing whitespace
makehuman.py:42: [E0602(undefined-variable), ] Undefined variable 'reload'
makehuman.py:43: [E1101(no-member), ] Module 'sys' has no 'setdefaultencoding' member
makehuman.py:45: [C0413(wrong-import-position), ] Import "import os" should be placed at the top of the module
makehuman.py:46: [C0413(wrong-import-position), ] Import "import re" should be placed at the top of the module
makehuman.py:47: [C0413(wrong-import-position), ] Import "import subprocess" should be placed at the top of the module
makehuman.py:156: [W0621(redefined-outer-name), unicode] Redefining name 'sys' from outer scope (line 41)
makehuman.py:136: [W0702(bare-except), unicode] No exception type(s) specified
makehuman.py:141: [W0702(bare-except), unicode] No exception type(s) specified
makehuman.py:146: [W0702(bare-except), unicode] No exception type(s) specified
makehuman.py:152: [W0702(bare-except), unicode] No exception type(s) specified
makehuman.py:158: [W0702(bare-except), unicode] No exception type(s) specified
makehuman.py:156: [W0404(reimported), unicode] Reimport 'sys' (imported line 41)
makehuman.py:163: [W0702(bare-except), unicode] No exception type(s) specified
makehuman.py:127: [R0911(too-many-return-statements), unicode] Too many return statements (7/6)
makehuman.py:191: [W0702(bare-except), get_revision_hg_info] No exception type(s) specified
makehuman.py:195: [W0613(unused-argument), get_revision_dirstate_parent] Unused argument 'folder'
makehuman.py:231: [W0613(unused-argument), get_revision_cache_tip] Unused argument 'folder'
makehuman.py:248: [E0401(import-error), get_revision_hglib] Unable to import 'hglib'
makehuman.py:275: [W0703(broad-except), get_hg_revision_1] Catching too general exception Exception
makehuman.py:276: [W0106(expression-not-assigned), get_hg_revision_1] Expression "((print) >> (sys.stderr), (('NOTICE: Failed to get hg version number from command line: ') + (format(unicode(e)))) + (" (This is just a head'
s up, not a critical error)"))" is assigned to nothing
makehuman.py:286: [W0703(broad-except), get_hg_revision_1] Catching too general exception Exception
makehuman.py:287: [W0106(expression-not-assigned), get_hg_revision_1] Expression "((print) >> (sys.stderr), (('NOTICE: Failed to get hg version number using hglib: ') + (format(unicode(e)))) + (" (This is just a head's up,
not a critical error)"))" is assigned to nothing
makehuman.py:295: [W0703(broad-except), get_hg_revision_1] Catching too general exception Exception
makehuman.py:296: [W0106(expression-not-assigned), get_hg_revision_1] Expression "((print) >> (sys.stderr), (('NOTICE: Failed to get hg parent version from dirstate file: ') + (format(unicode(e)))) + (" (This is just a head
's up, not a critical error)"))" is assigned to nothing
makehuman.py:304: [W0703(broad-except), get_hg_revision_1] Catching too general exception Exception
makehuman.py:305: [W0106(expression-not-assigned), get_hg_revision_1] Expression "((print) >> (sys.stderr), (('NOTICE: Failed to get hg tip version from cache file: ') + (format(unicode(e)))) + (" (This is just a head's up,
 not a critical error)"))" is assigned to nothing
makehuman.py:315: [W0105(pointless-string-statement), get_hg_revision_1] String statement has no effect
makehuman.py:331: [E0401(import-error), get_hg_revision] Unable to import 'getpath'
makehuman.py:335: [W0104(pointless-statement), get_hg_revision] Statement seems to have no effect
makehuman.py:340: [W0104(pointless-statement), get_hg_revision] Statement seems to have no effect
makehuman.py:343: [W0104(pointless-statement), get_hg_revision] Statement seems to have no effect
makehuman.py:371: [W0603(global-statement), get_platform_paths] Using the global statement
makehuman.py:372: [E0401(import-error), get_platform_paths] Unable to import 'getpath'
makehuman.py:385: [W0622(redefined-builtin), redirect_standard_streams] Redefining built-in 'open'
makehuman.py:401: [E0401(import-error), make_user_dir] Unable to import 'getpath'
makehuman.py:411: [E1101(no-member), init_logging] Module 'log' has no 'init' member
makehuman.py:412: [E1101(no-member), init_logging] Module 'log' has no 'message' member
makehuman.py:423: [W0703(broad-except), debug_dump] Catching too general exception Exception
makehuman.py:419: [W0106(expression-not-assigned), debug_dump] Expression "((print) >> (sys.stderr), ('Dependency error: ') + (format(unicode(e))))" is assigned to nothing
makehuman.py:425: [E1123(unexpected-keyword-arg), debug_dump] Unexpected keyword argument 'exc_info' in method call
makehuman.py:590: [W0212(protected-access), LicenseInfo.copy] Access to a protected member _customized of a client class
makehuman.py:620: [E1101(no-member), LicenseInfo.toNumpyString._packStringDict] Module 'numpy' has no 'uint32' member
makehuman.py:619: [R0204(redefined-variable-type), LicenseInfo.toNumpyString._packStringDict] Redefinition of text type from str to numpy.core.records.recarray
makehuman.py:662: [W0212(protected-access), getAssetLicense] Access to a protected member _customized of a client class
makehuman.py:707: [W0622(redefined-builtin), getSoftwareLicense] Redefining built-in 'open'
makehuman.py:706: [E0401(import-error), getSoftwareLicense] Unable to import 'getpath'
makehuman.py:725: [W0622(redefined-builtin), getThirdPartyLicenses] Redefining built-in 'open'
makehuman.py:724: [E0401(import-error), getThirdPartyLicenses] Unable to import 'getpath'
makehuman.py:727: [W0622(redefined-builtin), getThirdPartyLicenses._title] Redefining built-in 'license'
makehuman.py:766: [R0204(redefined-variable-type), getThirdPartyLicenses] Redefinition of external_licenses type from list to collections.OrderedDict
makehuman.py:801: [W0703(broad-except), main] Catching too general exception Exception
makehuman.py:802: [W0106(expression-not-assigned), main] Expression "((print) >> (sys.stderr), ('error: ') + (format(unicode(e))))" is assigned to nothing

pylint.log - Pylint against default 2017-03-12 (502 KB) Joel Palmius, 03/12/2017 03:12 PM

History

#1 Updated by Jonas Hauquier almost 2 years ago

Using static checkers to improve code quality is certainly a good idea. In the past I have used pylint and pychecker to clean up parts of MH. But always be suspicious when using such tools, use common sense and don't follow all advices blindly.
Also, I am not a supporter of following PEP guidelines, such as maximum character length per line for example, to the letter. Sometimes it just looks clearer to me when things are on one line.

#2 Updated by Rob Baer almost 2 years ago

Jonas Hauquier wrote:

... not a supporter of following PEP guidelines ... [to the letter]

And that is why they are just guidelines, not "rules" :-)

#3 Updated by Joel Palmius almost 2 years ago

Actually, the jenkins lint job is still running each night (to my surprise). The job is here http://jenkins.makehumancommunity.org/job/Lint_unstable/, but you need to log in and browse the workspace to find the lint log. Attaching tonight's log here.

The linter is currently run against default.

#4 Updated by Jonas Hauquier almost 2 years ago

Indeed, but using a tool such as pylint, one might be tempted to get rid of as many warnings as possible, which is not the best reflex in my opinion. :)

#5 Updated by Rob Baer almost 2 years ago

Agreed. Mindless anything is stupid.

In general, Pylint seems to produce C, W, And E level messages. I take the C to be code or comment and to be "suggestive", maybe like PEP8 compliance type messages, W to be warning messages and worth reading but not necessarily doing anything about depending on context, and E to be "errors" which should have a pretty good reason to leave in place even if they are not immediately threatening. We certainly should read them and think about them. Beyond that the secret sauce is in the execution.

Rather than "reacting to messages", maybe a sane approach would be to start by discussing "E level messages" on the Dev forum to understanding why they are appearing and the potential "gottcha's" they might present for the future? This discussion could prevent a lot of reckless anarchy and help the group stay on the same page going forward. That could focus us on cleaning up code rather than eradicating messages.

A message like:
[E0401, make_user_dir] Unable to import 'getpath'

would seem to indicate that our code is not written quite like we intended, but maybe it is no more than an artifact of the pylint engine runtime environment. Can't hurt to check it out.

Comments on this approach?

#6 Updated by Jonas Hauquier almost 2 years ago

Rob Baer wrote:

A message like:
[E0401, make_user_dir] Unable to import 'getpath'

would seem to indicate that our code is not written quite like we intended, but maybe it is no more than an artifact of the pylint engine runtime environment. Can't hurt to check it out.

It probably means that pylint does not get the path changes that we affect in the code.
Some time ago when I used pychecker I had a special bootstrap script to set up the paths so that it would find all imports.

#7 Updated by Joel Palmius almost 2 years ago

Some of the import errors are artifacts caused by us not using the standard python module layout with __init__.py files. Pylint can't really be expected to understand this automatically.

Anyway, I updated the pylint stuff for py3 too, and added it to the py3 repo. See here for first result https://github.com/makehumancommunity/makehuman_stable_python3/issues/19

One approach for filtering the log is to decide upon warnings/hints we think are irrelevant and add them to the "disable" clause in pylintrc (found in the makehuman dir in all repos, irregardless of py version).

Also available in: Atom