Python3 linter

[jngrad]

pylint was recently removed (b720b5c) because it only checked for one warning that had become a false positive in Python3. We can re-enable it (and move it to the style check job) if there is a need for rules that cannot be enforced by autopep8. A few candidates: class-naming-style, const-naming-style. Feel free to propose more rules here. The full list can be found in Pylint features.

[jngrad]

pylint can't run on Cython files, therefore whenever a .py file loads a function from a .pyx file, a warning is generated, usually one of E0001/E0401/E0611/E1101. The solution is to whitelist rules using --disable=all --enable=.... Most of the pylint logic has already been written down in the fix_style.sh script that runs in the style job, it's only a matter of reverting commit f2ea574 and optionally updating the python script that posts messages on GitHub to show the log. We should also pin the version of pylint in the dockerfile.

I think the following rules should always be enforced:

• W0102: mutable values in optional arguments: def foo(a=[]): to def foo(a=()):
• W0401: wildcard imports, e.g. from a import * to from a import (b, c, d\n e, f, g\n)
• W0611: unused imports
• W0614: unused imports from a wildcard
• W1505: deprecated assertRaisesRegexp()
• R1701: merge type checks: isinstance(x, y) or isinstance(x, z) to isinstance(x, (y, z))
• R1714: merge value checks: if x == 1 or x == 2: to if x in (1, 2):
• R1707: trailing comma tuple: return a, to either return a or return (a,)
• C0201: iterate dict keys directly
• C0202: use cls instead of self in class methods
• C0325: superfluous parentheses: if(a == 2): to if a == 2:
• W1652,W1651,W1649,W1657,W1660,W1658,W1659,W1623,W1622,W1620,W1621,W1645,W1624,W1625,
W1611,W1601,W1602,W1603,W1604,...: using Python2 features that have been deprecated or removed in Python3

The following rules should be run manually and periodically by maintainers to detect candidates for cleanup or refactoring:

• W0511: show todo
• W0612: unused variable
• W0613: unused arguments
• W1401: use raw strings in regex patterns
• R0401: cyclic imports
• R0912: too many branches
• R0915: too many statements
• R0916: too many boolean expressions
• R1702: too many nested blocks
• C0411: wrong ordering of import statements
• C1801: simplify empty list checks: if len(seq) > 0: to if seq:

To enable all rules except those failing on Cython or making too much noise: pylint3 --enable=all --disable=E0001,E0401,E0611,E1101,C0103,C0301,C0111,R0903,W1401,R0801,C0411,C0412,C0413,C0330,E1136,I src/ maintainer/ testsuite/

Other rules could pick up on PEP8 issues that are not properly handled by autopep8:

• W0301: trailing semicolons
• W0311: bad indentation
• C0303: trailing whitespaces
• C0321: more than one statement on a line
• C0327: mixed LF and CRLF line endings (difficult to spot in diffs)
• C0330: wrong hanging indentation
• C0414: useless import alias

[jngrad]

Rule C0411 detected 128 python files where import statements are incorrectly ordered. Cython and IPython files have the same issue. We could re-order them, just like we re-order C++ includes in the core to minimize merge conflicts. @KaiSzuttor found out fiximports might help automate this task.

[KaiSzuttor]

@KaiSzuttor found out fiximports might help automate this task.

isort looks more promising. see #3220

[jngrad]

With isort, the following constraint:

espresso/testsuite/python/h5md.py

Lines 28 to 30 in 8e549ad

	import espressomd # pylint: disable=import-error
	import h5py # h5py has to be imported *after* espressomd (MPI)
	from espressomd.interactions import Virtual

becomes:

import espressomd  # isort:skip; pylint: disable=import-error, wrong-import-order
import h5py  # isort:skip; pylint: disable=wrong-import-order; h5py has to be imported *after* espressomd (MPI)
from espressomd.interactions import Virtual  # isort:skip; pylint: disable=wrong-import-order,ungrouped-imports

[KaiSzuttor]

I think the following rules should always be enforced:
* W0102: mutable values in optional arguments: def foo(a=[]): to def foo(a=()):
* W0401: wildcard imports, e.g. from a import * to from a import (b, c, d\n e, f, g\n)
* W0611: unused imports
* W0614: unused imports from a wildcard
* W1505: deprecated assertRaisesRegexp()
* R1701: merge type checks: isinstance(x, y) or isinstance(x, z) to isinstance(x, (y, z))
* R1714: merge value checks: if x == 1 or x == 2: to if x in (1, 2):
* R1707: trailing comma tuple: return a, to either return a or return (a,)
* C0201: iterate dict keys directly
* C0202: use cls instead of self in class methods
* C0325: superfluous parentheses: if(a == 2): to if a == 2:

i fully agree with this list, plus I would add:

• W0612: unused variable
• W0613: unused arguments
• C1801: simplify empty list checks: if len(seq) > 0: to if seq:

@RudolfWeeber please comment on this aswell.

[RudolfWeeber]

I think there are quite a few trivial style checks in that list. I'm fine with the ones actually preventing dangerous stuff.

> I think the following rules should always be enforced: * `W1505`: deprecated `assertRaisesRegexp()`

This is quite common in the test suite.

* `R1701`: merge type checks: `isinstance(x, y) or isinstance(x, z)` to `isinstance(x, (y, z))`

Not dangerous.

* `R1714`: merge value checks: `if x == 1 or x == 2:` to `if x in (1, 2):`

Not dangerous

* `C0201`: iterate dict keys directly

Not dangerous.

* `C0202`: use `cls` instead of `self` in class methods

Not dangerous

* `C0325`: superfluous parentheses: `if(a == 2):` to `if a == 2:`

Dangerous

i fully agree with this list, plus I would add: * `W0612`: unused variable * `W0613`: unused arguments

I agree with those, since authors might expect changed behavior when changing a variable's value.

* `C1801`: simplify empty list checks: `if len(seq) > 0:` to `if seq:`

Not dangerous. In fact I find the 'if seq' more dangerous, because it will do something else if seq is not a sequence. The others are fine with me.

[KaiSzuttor]

so to summarize, we agree on the following rules:

• W0102: mutable values in optional arguments: def foo(a=[]): to def foo(a=()):
• W0401: wildcard imports, e.g. from a import * to from a import (b, c, d\n e, f, g\n)
• W0611: unused imports
• W0614: unused imports from a wildcard
• W1505: deprecated assertRaisesRegexp()
• R1707: trailing comma tuple: return a, to either return a or return (a,)
• C0325: superfluous parentheses: if(a == 2): to if a == 2:
• W0612: unused variable
• W0613: unused arguments
• W1401: use raw strings in regex patterns
• R0401: cyclic imports
• C0411: wrong ordering of import statements

[jngrad]

Maybe one more: E0602 no undefined variable/function (excluding samples/load_checkpoint.py and testsuite/python/test_checkpoint.py where variables are loaded implicitly when unpickling).

Maybe one less: C0411 wrong ordering of import statements, I think Rudolf wasn't convinced having unsorted import statements was a good reason for causing a failure in CI.

The corresponding pylint command would be: pylint3 --score=no --reports=no --disable=all --enable=W0102,W0401,W0611,W0614,W1505,R1707,C0325,W0612,W0613,W1401,R0401 src testsuite samples maintainer doc; running this on the jngrad:fix-2089 branch – where the samples have been cleaned up – outputs a reasonable list of fixes. There are a couple false positives like unused arguments *args, **kwargs in highlander.py or unused imports in tutorial 6 where students are expected to fill in blanks, but these can be disabled by pragmas.

@RudolfWeeber have you accidentally exchanged two items in your reply above? Specifically:

• C0202: use cls instead of self in class methods
  Not dangerous
• C0325: superfluous parentheses: if(a == 2): to if a == 2:
  Dangerous

I don't immediately see how if(a == 2): could be dangerous. However using self instead of cls is an anti-pattern:

class A():
    property = "before"
    @classmethod
    def foo(self, new_value):
        self.property = new_value  # this is not actually self!!

a = A()
b = A()
print(a.property)  # "before"
print(b.property)  # "before"
a.foo("after")
print(a.property)  # "after"
print(b.property)  # "after"

[RudolfWeeber]

@jngrad, the "not" in "not dangerous" went missing for
if (a ==2)
I indeed think, this is a not terribly important style thing.
I'm fine with the rest.

[jngrad]

This issue needs merging #3267 first.

[jngrad]

removing W1401: use raw strings, it has too many false positives, e.g.

• \*\*kwargs or :math:`f_{x1},\\ f_{y1}` in docstrings (asterisks and backslashes have to be escaped),
• re.sub(..., '\g<0>\n') (here we need an actual newline, not the two characters \ and n)

[jngrad]

versions of pylint found in the Ubuntu repositories have an outdated command line interface