-
-
Notifications
You must be signed in to change notification settings - Fork 18k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CLN/STYLE: Lint comprehensions #22075
Conversation
Syntax error in one of the edits. +1 on the concept. |
@@ -9,6 +9,7 @@ dependencies: | |||
- fastparquet | |||
- feather-format | |||
- flake8=3.4.1 | |||
- flake8-comprehensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to add this to the dev deps as well
Tons of flake complaints, may need to implement gradually. |
Now it’s mostly upset about lambdas. OTOH it’s reassuring to see mine isn’t the only PR that pytables is messing up. |
Is there a predefined set of lint codes we ignore? I am a little puzzled why these lint checks now started to fire; I figure it has something to do with the scope of checks when |
in setup.cfg |
Possibly for the while-were-at-it pile: #11954 checks for unnecessary backslashes (could this be done with just a grep?) |
we have a fixed version of flake8 (an open issue about this too) - which doesn’t check bare excepts |
pls rebase |
The travis failure is from the reoccuring |
thanks @mroeschke if you think need to open additional issues for the skipped pep codes, pls do. alternatively maybe just add some docs on them on why we need to keep them there (e.g. so next person isn't wondering why those particular codes). |
if [ $? -ne "0" ]; then | ||
RET=1 | ||
fi | ||
echo "Linting *.py DONE" | ||
|
||
echo "Linting setup.py" | ||
flake8 setup.py | ||
flake8 setup.py --ignore=C405,C406,C408,C409,C410,E402,E731,E741,W503 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. here if you'd document why these codes (general comments are enough), though if specific ones can be removed pls do that. also maybe want to add these to setup.cfg
?
@@ -6,6 +6,7 @@ dependencies: | |||
- Cython>=0.28.2 | |||
- NumPy | |||
- flake8 | |||
- flake8-comprehensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think need to run and commit scripts/convert_deps.py to have requirements_dev.txt updated. can you PR that?
git diff upstream/master -u -- "*.py" | flake8 --diff
Using flake8-comprehensions (xref #20588 (comment)) to lint for unnecessary comprehensions. I just added this to the 2.7-travis build since it appears to be our linting build.