Enable flake8 warnings for all currently non-violated rules #12295

• Enable flake8 warnings for all currently non-violated rules
• Fix accidental redefinition via list comprehension

This should probably be reviewed by @jnewbery, @MarcoFalke and @MeshCollider :-)

This is an improvement in style, but it doesn't change behaviour (tx is not used after this line)

I'm not sure if it makes sense to open PRs to fix individual flake8 violations. By my count, there are currently >5000:

→ flake8 test/functional/*py | wc -l
5101

Fixing them in PRs like this will generate a huge amount of review work.

@jnewbery Agreed – fixing all flake8 violations would clearly be a waste of time.

And yes, we were lucky this time that tx was never used again after the redefinition. But why count on luck? .-)

Redefinition via list comprehension is a real and recurring gotcha. Enabling the check for it in python-lint.sh and thus getting a guarantee that we'll never be bitten by this again seems like an easy win to me :-)

Agreed – fixing all flake8 violations would clearly be a waste of time.

Redefinitions via list comprehensions is a real and recurring gotcha

Agreed that some flake8 warnings are more useful than others. Rather than have a PR for just one of the dangerous ones, perhaps it makes sense to have a single PR that adds all of the important checks?

The changes here seem fine, but I don't want to open the floodgates to "add flake8 waning ???? to Travis linting" PRs.

Marco is the test maintainer. Let's see what he thinks the correct approach is.

@jnewbery For the flake8 violations we currently have in the code base I'd say that fixing and enabling Travis checking for F841 (PR #12284: local variable 'foo' is assigned to but never used) and F812 (this PR #12295: list comprehension redefines 'foo' from line N ) would be enough.

If we want to go beyond that I'd suggest that we consider enabling the checks that we currently do not violate in our code base. Enabling them would guarantee that we will never introduce any new classes of violations in the future. FWIW, the following flake8 command does not emit any warnings on master:

$ git checkout master
$ git pull upstream master
$ flake8 --ignore=B,C,E,F,I,N,W --select=F402,F404,F406,F407,F601,F602,F621,F622,F631,\
    F701,F702,F703,F704,F705,F706,F707,F811,F822,F823,F831,E112,E113,E115,E116,E125,\
    E131,E133,E223,E224,E271,E272,E273,E274,E275,E304,E306,E502,E702,E703,\
    E714,E721,E741,E742,E743,W292,W504,W601,W602,W603,W604,W605 .
$

These refer to:

F402 import module from line N shadowed by loop variable
F404 future import(s) name after other statements
F406 ‘from module import *’ only allowed at module level
F407 an undefined __future__ feature name was imported
F601 dictionary key name repeated with different values
F602 dictionary key variable name repeated with different values
F621 too many expressions in an assignment with star-unpacking
F622 two or more starred expressions in an assignment (a, *b, *c = d)
F631 assertion test is a tuple, which are always True
F701 a break statement outside of a while or for loop
F702 a continue statement outside of a while or for loop
F703 a continue statement in a finally block in a loop
F704 a yield or yield from statement outside of a function
F705 a return statement with arguments inside a generator
F706 a return statement outside of a function/method
F707 an except: block as not the last exception handler
F811 redefinition of unused name from line N
F822 undefined name name in __all__
F823 local variable name … referenced before assignment
F831 duplicate argument name in function definition
E112 expected an indented block
E113 unexpected indentation
E115 expected an indented block (comment)
E116 unexpected indentation (comment)
E125 continuation line with same indent as next logical line
E131 continuation line unaligned for hanging indent
E133 closing bracket is missing indentation
E223 tab before operator
E224 tab after operator
E271 multiple spaces after keyword
E272 multiple spaces before keyword
E273 tab after keyword
E274 tab before keyword
E275 missing whitespace after keyword
E304 blank lines found after function decorator
E306 expected 1 blank line before a nested definition
E502 the backslash is redundant between brackets
E702 multiple statements on one line (semicolon)
E703 statement ends with a semicolon
E714 test for object identity should be ‘is not’
E721 do not compare types, use ‘isinstance()’
E741 do not use variables named ‘l’, ‘O’, or ‘I’
E742 do not define classes named ‘l’, ‘O’, or ‘I’
E743 do not define functions named ‘l’, ‘O’, or ‘I’
W292 no newline at end of file
W504 line break after binary operator
W601 .has_key() is deprecated, use ‘in’
W602 deprecated form of raising exception
W603 ‘<>’ is deprecated, use ‘!=’
W604 backticks are deprecated, use ‘repr()’
W605 invalid escape sequence ‘x’

Concept ACK on enabling the warning. However, I agree with @jnewbery that enabling and "fixing" each warning in a separate pull will generate too much noise.

Rather than have a PR for just one of the dangerous ones, perhaps it makes sense to have a single PR that adds all of the important checks?

Makes sense.

I will take a look at the list above at some point...

@MarcoFalke I've now renamed tx to dec_tx as requested. Would you mind re-reviewing? :-)

I read through the list of suggested lint warnings and I couldn't find an obvious wrong one. Also, since you claim that none of these are currently hit on master, I think it is fine to enable all of them in one go. Mind to repurpose this pull request for that?

@MarcoFalke Great idea! Now updated. Please review :-)

Cool. Thx.

utACK a9d0ebc26207b4771b7c240ca0c516debd330985

Concept ACK / light review utACK, I think if we can add them without much churn then its sane, agree with above about fixing all 5000+ warnings though :)

utACK a9d0ebc26207b4771b7c240ca0c516debd330985