NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.4k

Run flake8 instead of pep8 on Python 3.6#10940

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

cclauss wants to merge2 commits intomatplotlib:masterfromcclauss:patch-1

Closed

Run flake8 instead of pep8 on Python 3.6#10940

cclauss wants to merge2 commits intomatplotlib:masterfromcclauss:patch-1

Conversation

Copy link

cclauss commentedApr 2, 2018•
edited
Loading

As discussed at#10938 (comment)undefined names haveoccurred often in the past and this change would help to avoid recurrence in the future. Flake8 is a superset of pycodestyle (fka PEP8) so we do not need to run both.

PR Summary

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

anntzer reviewed

Apr 2, 2018

View reviewed changes

.flake8 Outdated

		@@ -0,0 +1,2 @@
		[flake8]

Copy link

Contributor

anntzerApr 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ithink the config can also live in pytest.ini (not sure)? That'd help avoid ending with a gazillion config files in the root folder. Also because it makes sense to keep that next to the pep8ignore list.

Copy link

Author

cclaussApr 2, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

flake8 is a superset of pep8 (renamed to pycodestyle at the BDFL's request) so if we run flake8, we no longer need to run pycodestyle.

anntzer reviewed

Apr 2, 2018

View reviewed changes

.travis.yml Outdated

		@@ -82,7 +82,7 @@ matrix:
		- python: 3.5
		env: PYTHON_ARGS=-OO
		- python: 3.6
		env: DELETE_FONT_CACHE=1 PANDAS='pandas<0.21.0' PYTEST_PEP8=pytest-pep8 RUN_PEP8=--pep8
		env: DELETE_FONT_CACHE=1 PANDAS='pandas<0.21.0' PYTEST_PEP8=pytest-flake8 RUN_PEP8=--flake8

Copy link

Contributor

anntzerApr 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

RUN_FLAKE8

cclauss force-pushed thepatch-1 branch 2 times, most recently fromfc04cbe to241f11fCompare

April 2, 2018 11:28

Copy link

Author

cclauss commentedApr 2, 2018

@anntzer Ready for review.

tacaswell added this to thev3.0 milestone

Apr 2, 2018

tacaswell added the topic: testing label

Apr 2, 2018

Copy link

Member

tacaswell commentedApr 2, 2018

👍 in principle, but some questions.

Does flake8 inherit the pep8 configuration? If not we are going to need the per-file filters.

Are the error codes shared between the two? If so, why is the ignore list different?

Can you also remove all of theRUN_PEP8 related code from the .travis file?

Copy link

Author

cclauss commentedApr 2, 2018•
edited
Loading

The Exxx codes are shared butC901 and the Fxxx codes only exist inflake8 for instance.

ALL Python files currently pass with the current flake8-ignore list.My sense is that this is a better approach then per-file exceptions because there are several codes that could be removed with minor edits to just a few files. We could migrate to something like Django’s zero tolerance.

I will remove the PEP8 cruft later today.

Copy link

Member

tacaswell commentedApr 2, 2018

Please keep the per-file exception list and do not change the Exxx codes that we are ignoring. Going through and changing working code for purely stylistic reasons is un-needed code-churn and we have had several instances where it has introduced bugs. Having per-file exception lists lets us enforce checks on most files without having to go through fix the handful of failing files.

Please set the line check back to 80.

I am 👍 on getting the extra checks from pyflakes, I am strongly 👎 on changing what aspect of pycodestyle we are enforcing.

cclauss force-pushed thepatch-1 branch 3 times, most recently from5aa8afc to7552aeeCompare

April 5, 2018 14:06

dstansby previously approved these changes

Apr 7, 2018

View reviewed changes

dopplershift requested changes

Apr 9, 2018

View reviewed changes

Copy link

Contributor

dopplershift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Doesn't this need to doRUN_PEP8->RUN_FLAKE8 intest_script.sh to actually run flake8?

Copy link

Member

QuLogic commentedApr 10, 2018

@dopplershift is right; onmaster we have 7384+28+12+9=7433 tests on 3.5 and 8213+8+12+9=8242 tests on 3.6, but here we have 7378+28+12+9=7427 tests on 3.5 and 7407+8+12+9=7436 tests on 3.6. That is, there should be ~800 extra tests like there is withmaster.

dstansby dismissed theirstale review

April 10, 2018 10:48

Woops I shouldn't have approved this

Run flake8 instead of pep8 on Python 3.6

5c77587

As discussed atmatplotlib#10938 (comment) _undefined names_ have [occurred often in the past](https://github.com/matplotlib/matplotlib/pulls?q=is%3Apr+author%3Acclauss+is%3Aclosed) and this change would help to avoid recurrence in the future.

cclauss force-pushed thepatch-1 branch 3 times, most recently fromf4c6cf1 to27dc890Compare

May 8, 2018 11:11

Restore per-file exceptions, fix test_script.sh

369da09

cclauss force-pushed thepatch-1 branch from27dc890 to369da09Compare

May 8, 2018 12:00

Copy link

Author

cclauss commentedMay 8, 2018•
edited
Loading

This update restores the theper-file exception list as requested and adds the followingexceptions:

E305 - expected 2 blank lines after end of function or class
E306 - expected 1 blank line before a nested definition
E722 - do not use bare except, specify exception instead
E741 - do not use variables named ‘l’, ‘O’, or ‘I’
F401 -module imported but unused
F403 - 'frommodule import *’ used; unable to detect undefined names
F811 - redefinition ofunused name from lineN
F841 - local variablename is assigned to but never used

RUN_FLAKE8 is now run intest_script.sh and the number of tests now matches master.

The E7's and F4's might be worth fixing in future pull requests.

anntzer reviewed

May 24, 2018

View reviewed changes

.travis.yml

		- SPHINX=sphinx
		# Variables controlling the test run.
		- DELETE_FONT_CACHE=
		- NO_AT_BRIDGE=1 # Necessary for GTK3 interactive test.
		- NPROC=2
		- OPENBLAS_NUM_THREADS=1
		- PYTHONFAULTHANDLER=1
		- PYTEST_ARGS="-rawR --maxfail=50 --timeout=300 --durations=25 --cov-report= --cov=lib -n $NPROC"
		- RUN_FLAKE8=

Copy link

Contributor

anntzerMay 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

please move it down (it's in alphabetical order)

Copy link

Contributor

anntzer commentedMay 24, 2018

I will approve the PR after a rebase (and the minor comment above). I think it is clear that we are constantly introducing bugs in untested paths without that...

The added error codes are all reasonable and are only present in pycodestyle, not pep8, so that's why they're needed.