Closed jngrad closed 4 years ago
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 importsW0614
: unused imports from a wildcardW1505
: 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 directlyC0202
: use cls
instead of self
in class methodsC0325
: 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 Python3The following rules should be run manually and periodically by maintainers to detect candidates for cleanup or refactoring:
W0511
: show todoW0612
: unused variableW0613
: unused argumentsW1401
: use raw strings in regex patternsR0401
: cyclic importsR0912
: too many branchesR0915
: too many statementsR0916
: too many boolean expressionsR1702
: too many nested blocksC0411
: wrong ordering of import statementsC1801
: 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 semicolonsW0311
: bad indentationC0303
: trailing whitespacesC0321
: more than one statement on a lineC0327
: mixed LF and CRLF line endings (difficult to spot in diffs)C0330
: wrong hanging indentationC0414
: useless import aliasRule 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 found out fiximports might help automate this task.
isort looks more promising. see #3220
With isort
, the following constraint:
https://github.com/espressomd/espresso/blob/8e549adbbbc849b2cf92d21747c26a4a7ac61c53/testsuite/python/h5md.py#L28-L30
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
I think the following rules should always be enforced:
W0102
: mutable values in optional arguments:def foo(a=[]):
todef foo(a=()):
W0401
: wildcard imports, e.g.from a import *
tofrom a import (b, c, d\n e, f, g\n)
W0611
: unused importsW0614
: unused imports from a wildcardW1505
: deprecatedassertRaisesRegexp()
R1701
: merge type checks:isinstance(x, y) or isinstance(x, z)
toisinstance(x, (y, z))
R1714
: merge value checks:if x == 1 or x == 2:
toif x in (1, 2):
R1707
: trailing comma tuple:return a,
to eitherreturn a
orreturn (a,)
C0201
: iterate dict keys directlyC0202
: usecls
instead ofself
in class methodsC0325
: superfluous parentheses:if(a == 2):
toif a == 2:
i fully agree with this list, plus I would add:
W0612
: unused variableW0613
: unused argumentsC1801
: simplify empty list checks: if len(seq) > 0:
to if seq:
@RudolfWeeber please comment on this aswell.
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
: deprecatedassertRaisesRegexp()
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:
toif 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.
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 importsW0614
: unused imports from a wildcardW1505
: 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 variableW0613
: unused argumentsW1401
: use raw strings in regex patternsR0401
: cyclic importsC0411
: wrong ordering of import statementsMaybe 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
: usecls
instead ofself
in class methods Not dangerousC0325
: superfluous parentheses:if(a == 2):
toif 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"
@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.
This issue needs merging #3267 first.
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
)versions of pylint found in the Ubuntu repositories have an outdated command line interface
pylint
was recently removed (b720b5c1) 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 byautopep8
. 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.