Run flake8 instead of pep8 on Python 3.6 #10940

cclauss · 2018-04-02T05:28:03Z

As discussed at #10938 (comment) undefined names have occurred 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 · 2018-04-02T07:43:59Z

.flake8

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


I think 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.

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 · 2018-04-02T07:44:33Z

.travis.yml

@@ -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


cclauss · 2018-04-02T11:29:08Z

@anntzer Ready for review.

tacaswell · 2018-04-02T11:54:00Z

👍 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 the RUN_PEP8 related code from the .travis file?

cclauss · 2018-04-02T12:18:49Z

The Exxx codes are shared but C901 and the Fxxx codes only exist in flake8 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.

tacaswell · 2018-04-02T14:20:06Z

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.

dopplershift

Doesn't this need to do RUN_PEP8-> RUN_FLAKE8 in test_script.sh to actually run flake8?

QuLogic · 2018-04-10T04:54:46Z

@dopplershift is right; on master 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 with master.

Woops I shouldn't have approved this

As discussed at matplotlib#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 · 2018-05-08T13:08:25Z

This update restores the the per-file exception list as requested and adds the following exceptions:

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 - 'from module import *’ used; unable to detect undefined names
F811 - redefinition of unused name from line N
F841 - local variable name is assigned to but never used

RUN_FLAKE8 is now run in test_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 · 2018-05-24T07:56:46Z

.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=


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

anntzer · 2018-05-24T08:01:18Z

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.

QuLogic · 2018-06-22T03:05:14Z

Rebased in #11477; thanks for the initial work on this @cclauss.

anntzer reviewed Apr 2, 2018

View reviewed changes

cclauss force-pushed the patch-1 branch 2 times, most recently from fc04cbe to 241f11f Compare April 2, 2018 11:28

tacaswell added this to the v3.0 milestone Apr 2, 2018

tacaswell added the topic: testing label Apr 2, 2018

cclauss force-pushed the patch-1 branch 3 times, most recently from 5aa8afc to 7552aee Compare 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

Run flake8 instead of pep8 on Python 3.6

5c77587

As discussed at matplotlib#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 the patch-1 branch 3 times, most recently from f4c6cf1 to 27dc890 Compare May 8, 2018 11:11

Restore per-file exceptions, fix test_script.sh

369da09

cclauss force-pushed the patch-1 branch from 27dc890 to 369da09 Compare May 8, 2018 12:00

anntzer reviewed May 24, 2018

View reviewed changes

anntzer mentioned this pull request Jun 12, 2018

nbagg broken on master. 'Path' is not defined... #11426

Closed

QuLogic mentioned this pull request Jun 22, 2018

Enable flake8 and re-enable it everywhere #11477

Merged

1 task

QuLogic closed this Jun 22, 2018

QuLogic added the status: duplicate label Jun 22, 2018

cclauss deleted the patch-1 branch June 22, 2018 04:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run flake8 instead of pep8 on Python 3.6 #10940

Run flake8 instead of pep8 on Python 3.6 #10940

cclauss commented Apr 2, 2018 •

edited

Loading

anntzer Apr 2, 2018

cclauss Apr 2, 2018 •

edited

Loading

anntzer Apr 2, 2018

cclauss commented Apr 2, 2018

tacaswell commented Apr 2, 2018

cclauss commented Apr 2, 2018 •

edited

Loading

tacaswell commented Apr 2, 2018

dopplershift left a comment

QuLogic commented Apr 10, 2018

cclauss commented May 8, 2018 •

edited

Loading

anntzer May 24, 2018

anntzer commented May 24, 2018

QuLogic commented Jun 22, 2018

Run flake8 instead of pep8 on Python 3.6 #10940

Run flake8 instead of pep8 on Python 3.6 #10940

Conversation

cclauss commented Apr 2, 2018 • edited Loading

PR Summary

PR Checklist

anntzer Apr 2, 2018

Choose a reason for hiding this comment

cclauss Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

anntzer Apr 2, 2018

Choose a reason for hiding this comment

cclauss commented Apr 2, 2018

tacaswell commented Apr 2, 2018

cclauss commented Apr 2, 2018 • edited Loading

tacaswell commented Apr 2, 2018

dopplershift left a comment

Choose a reason for hiding this comment

QuLogic commented Apr 10, 2018

cclauss commented May 8, 2018 • edited Loading

anntzer May 24, 2018

Choose a reason for hiding this comment

anntzer commented May 24, 2018

QuLogic commented Jun 22, 2018

cclauss commented Apr 2, 2018 •

edited

Loading

cclauss Apr 2, 2018 •

edited

Loading

cclauss commented Apr 2, 2018 •

edited

Loading

cclauss commented May 8, 2018 •

edited

Loading