mwaskom
commented
6 years ago Going forward should probably update to pycodestyle (which replaced the pep8 script) or even better, flake8 to get linting too.
Closed djarecka closed 6 years ago
mwaskom
commented
6 years ago Going forward should probably update to pycodestyle (which replaced the pep8 script) or even better, flake8 to get linting too.
effigies
commented
6 years ago :+1: to flake8. Not worth it until we lint the whole tree, but it's pretty easy to add a Travis row to run flake8 on all PRs.
mwaskom
commented
6 years ago Here's an idea of the most common problems:
10476 E124 closing bracket does not match visual indentation
10225 E501 line too long (82 > 79 characters)
10178 E122 continuation line missing indentation or outdented
8833 E128 continuation line under-indented for visual indent
1319 F401 'nipype' imported but unused
249 E402 module level import not at top of file
209 E231 missing whitespace after ','
148 E302 expected 2 blank lines, found 0
96 E127 continuation line over-indented for visual indent
95 F405 'skipif' may be undefined, or defined from star imports: numpy.testing.decorators
78 E303 too many blank lines (2)
64 E305 expected 2 blank lines after class or function definition, found 1
61 F841 local variable 'tblexer' is assigned to but never used
60 E701 multiple statements on one line (colon)
58 W391 blank line at end of file
58 F811 redefinition of unused 'fsl' from line 24
46 E265 block comment should start with '# '
44 E111 indentation is not a multiple of four
43 E261 at least two spaces before inline comment
40 E203 whitespace before ':'
31 E731 do not assign a lambda expression, use a def
22 F403 'from .diffusion import *' used; unable to detect undefined names
17 E711 comparison to None should be 'if cond is None:'
16 F821 undefined name 'freesurfer'
15 E502 the backslash is redundant between brackets
15 E221 multiple spaces before operator
14 E251 unexpected spaces around keyword / parameter equals
13 E225 missing whitespace around operator
11 E262 inline comment should start with '# '
11 E131 continuation line unaligned for hanging indent
9 E712 comparison to True should be 'if cond is True:' or 'if cond:'
7 E401 multiple imports on one line
6 W191 indentation contains tabs
5 E713 test for membership should be 'not in'
5 E272 multiple spaces before keyword
5 E211 whitespace before '('
5 E201 whitespace after '('
4 E301 expected 1 blank line, found 0
4 E202 whitespace before ')'
3 E228 missing whitespace around modulo operator
3 E222 multiple spaces after operator
2 E125 continuation line with same indent as next logical line
2 E101 indentation contains mixed spaces and tabs
1 W292 no newline at end of file
1 F402 import 'op' from line 52 shadowed by loop variable
1 E714 test for object identity should be 'is not'
1 E306 expected 1 blank line before a nested definition, found 0
1 E271 multiple spaces after keyword
1 E266 too many leading '#' for block comment
1 E129 visually indented line with same indent as next logical line
djarecka
commented
6 years ago updated the list to the flake8 output
miykael
commented
6 years ago @mwaskom's list also includes test_auto*.py files. To get the same list without those files, one can use the following command: flake8 -qq --filename=*py --exclude=test_auto* --statistics | sort -rh
The output for this command looks as follows:
10002 E501 line too long (82 > 79 characters)
1319 F401 'nipype' imported but unused
265 E402 module level import not at top of file
209 E231 missing whitespace after ','
176 E128 continuation line under-indented for visual indent
147 E302 expected 2 blank lines, found 1
96 E127 continuation line over-indented for visual indent
95 F999 MRIConvert may be undefined, or defined from star imports: interfaces.freesurfer
78 E303 too many blank lines (2)
72 F841 local variable 'pickfirst' is assigned to but never used
60 E701 multiple statements on one line (colon)
58 F811 redefinition of unused 'fsl' from line 24
52 E124 closing bracket does not match visual indentation
46 E265 block comment should start with '# '
44 E111 indentation is not a multiple of four
43 E261 at least two spaces before inline comment
40 E203 whitespace before ':'
31 E731 do not assign a lambda expression, use a def
23 W391 blank line at end of file
22 F403 'from interfaces.freesurfer import *' used; unable to detect undefined names
17 E711 comparison to None should be 'if cond is not None:'
16 F821 undefined name 'freesurfer'
15 E502 the backslash is redundant between brackets
15 E221 multiple spaces before operator
14 E251 unexpected spaces around keyword / parameter equals
13 E225 missing whitespace around operator
11 E262 inline comment should start with '# '
11 E131 continuation line unaligned for hanging indent
9 E712 comparison to True should be 'if cond is True:' or 'if cond:'
7 E401 multiple imports on one line
6 W191 indentation contains tabs
5 E713 test for membership should be 'not in'
5 E301 expected 1 blank line, found 0
5 E272 multiple spaces before keyword
5 E211 whitespace before '('
5 E201 whitespace after '{'
5 E122 continuation line missing indentation or outdented
4 E202 whitespace before ')'
3 W503 line break before binary operator
3 E228 missing whitespace around modulo operator
3 E222 multiple spaces after operator
2 E125 continuation line with same indent as next logical line
2 E101 indentation contains mixed spaces and tabs
1 W292 no newline at end of file
1 F402 import 'op' from line 52 shadowed by loop variable
1 E714 test for object identity should be 'is not'
1 E271 multiple spaces after keyword
1 E266 too many leading '#' for block comment
1 E129 visually indented line with same indent as next logical lineIs anybody already working on this? I'm happy to dig my teeth into this, but I don't want to interfere with somebody else. If I don't here from anybody in the next few days, than I will start cleaning out this list.
djarecka
commented
6 years ago do we want to keep the default 79 character per line or we want to allow longer lines?
miykael
commented
6 years ago I'm not sure if it makes sense to increase the default of 79 as it is an industry standard. But for completeness, the number of errors would decrease with characters per line as follows:
79 characters allowed per line = 9996 E501 errors
89 characters allowed per line = 5896 E501 errors
99 characters allowed per line = 4063 E501 errors
109 characters allowed per line = 2896 E501 errorsSo, I think the issue is kind of persisting. I would rather first try to correct the E501 errors before changing the standard.
As for example in https://github.com/nipy/nipype/blob/master/nipype/interfaces/fsl/fix.py, this file goes up to 284 characters per line and has a total of nine E501.
miykael
commented
6 years ago I tried with https://github.com/nipy/nipype/pull/2358 to correct most PEP8 violations (except E501).
Here's a list of the violations that remain and the reason why they haven't been corrected yet:
Most of the following pyflakes/flake8 errors were not touched, as their handling often implies rather strong code changes/refactoring. For completness, here the name and number of the still open errors:
F401 (N=1347) 'nipype' imported but unused
F999 (N=89) cd may be undefined, or defined from star imports: toollib
F811 (N=49) redefinition of unused 'pd' from line 23
F841 (N=33) local variable 'newfiles' is assigned to but never used
F403 (N=20) 'from toollib import *' used; unable to detect undefined names
F821 (N=16) undefined name 'freesurfer'I also didn't attack E402 (N=259) module level import not at top of file , as most of those issues are caused by standard_library.install_aliases() (example) or are coming from a workflow example, where the import often isn't at the beginning of the file.
I also didn't attack 30 errors of type E265 block comment should start with '# ', as they were all in the conf.py file, where this kind of format is prefered.
I also didn't correct 29 errors of the type E731 do not assign a lambda expression, use a def as their correction would mean code modification that are outside the scope of just style correction.
The file nipype/interfaces/freesurfer/tests/test_FSSurfaceCommand.py seems to be auto-generated, but the filename doesn't have the prefix test_auto_. Because of that, this file leads to the following 10 violations:
E122 (N=3) continuation line missing indentation or outdented
E124 (N=4) closing bracket does not match visual indentation
E128 (N=3) continuation line under-indented for visual indent@effigies, is this file still auto-generated, or can I correct the errors manually?
djarecka
commented
6 years ago @miykael - i don't think test_FSSurfaceCommand.py is auto-generated. it looks like @effigies created this file based on the old auto-test, but it should be now changed manually
miykael
commented
6 years ago @djarecka - ok, great. I updated the file with https://github.com/nipy/nipype/pull/2358/commits/730c1a12eca6a4a49190eee3b549c38152396bd7 to correct the pep8 violations.
effigies
commented
6 years ago Ah, sorry, I missed this buried in the huge pile of notifications coming out of nipype. Yes that is a modified auto test. If there's a better way to organize things so that we don't run into this in the future, let me know. (And maybe it would be good to have in a doc on testing practices.)
djarecka
commented
6 years ago I'm closing this issue now. If there is something more to fix right now, please open a new issue.
I'm closing an old issue #597 and providing a new list of files that returns errors and warnings with
flake8.Below is the output of
for py3. note that I removed auto tests that should be changed in
checkspecs.py:Everyone is welcome to update the files from the list so we're closer to the pep8 standard.
Please leave a comment here if you believe that some error shouldn't/can't be corrected (I saw some old comments in #1264 , but not sure if they are still relevant.