Enable flake8 and re-enable it everywhere #11477

QuLogic · 2018-06-22T03:04:34Z

PR Summary

Due to the removal of the examples symlink in #11141, pytest was no longer seeing the examples and running pep8 on them. This adds that directory back to pytest's search.

It also adds a rebase of #10940 to enable flake8.

PR Checklist

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

QuLogic · 2018-06-22T03:16:27Z

The meaning of the additional exceptions to flake8 options are described here by @cclauss.

anntzer · 2018-06-22T07:11:24Z

pytest.ini

@@ -7,8 +7,9 @@ markers =
    network: Mark a test that uses the network.
    style: Set alternate Matplotlib style temporarily.

-pep8ignore =
-    * E111 E114 E115 E116 E121 E122 E123 E124 E125 E126 E127 E128 E129 E131 E226 E240 E241 E242 E243 E244 E245 E246 E247 E248 E249 E265 E266 E704 W503
+flake8-max-line-length = 80


Shouldn't this be left at 79 (the default, i.e. remove this line)?

I got this from @cclauss's PR, but I think you're right, so I'm going to remove it.

sorry, off-by-one bug on my part.

cclauss · 2018-06-22T08:38:43Z

LOL... #10940 (comment)

QuLogic · 2018-06-23T20:49:06Z

Well, okay, now pyside(2) seems to be broken.

Also, I have no idea why, but flake8 make the tests ridiculously slow. I ran a non-parallel test locally and it took ~7 minutes, but with the --flake8 flag it's not finished for the past hour.

timhoffm · 2018-06-23T22:28:55Z

Maybe related tholo/pytest-flake8#19?

anntzer · 2018-07-02T20:07:26Z

If #11477 (comment) is still an issue, perhaps replace pytest-flake8 by a direct invocation of flake8? (with e.g. https://github.com/snoack/flake8-per-file-ignores for per-file ignores)
(TBH I don't really see the point of a pytest plugin that just calls another executable (if it is not active by default).)

jklymak · 2018-07-06T20:58:57Z

I ran

pytest -v --pep8 lib/matplotlib/tests/test_axes.py: 143.09 s
pytest -v --flake8 lib/matplotlib/tests/test_axes.py: 131.09 s

Sure I didn't have the exceptions turned on or anything (the pep8 test failed).

cclauss · 2018-07-06T21:28:06Z

Try running PEP8 and flake8 WITHOUT passing thru pytest.

QuLogic · 2018-07-07T09:38:45Z

Well, I tried conda, virtualenv, and even downloading the docker image that Travis used, but I cannot reproduce the slow checks anymore. I guess standalone flake8 is the only option; the only annoyance is that configuration will have to go into a new file, as pytest.ini is not a supported file.

cclauss · 2018-07-07T11:11:07Z

From http://flake8.pycqa.org/en/latest/user/invocation.html

$ flake8 . --config=CONFIG # Path to the config file that will be the authoritative config source. This will cause Flake8 to ignore all other configuration files.

tacaswell · 2018-07-07T18:08:01Z

Is the issue that pytest spins up a new flake8 process for each file which just takes forever on the travis vms?

tacaswell · 2018-07-07T18:10:47Z

watching a running test it is not hung, just from 91% on it seems to be taking it's time (and then we hit the 50 minute timeout).

cclauss · 2018-07-07T20:33:38Z

One minute, 10 seconds to flake8 test the entire repo seems reasonable to me.

$ time flake8 . --config=pytest.ini # Python 3.7 on Travis CI

real	0m26.769s
user	1m10.204s
sys	0m0.953s

cclauss

Running flake8 from inside pytest has proved to be a bad idea when each file in the repo has its own style rules. Running flake8 standalone with the --config=pytest.ini seems the best approach.

QuLogic · 2018-07-07T23:59:52Z

At some point in the near future, I would like to get rid of the ** exceptions, as they cause confusion when linting a single file (per-file-ignores thinks that the exception is unnecessary and raises its X100 error).

jklymak · 2018-07-08T00:39:43Z

Where does the flake8 output show up in the test? I can't see any evidence that it ran, but maybe I'm looking in the wrong spot...

QuLogic · 2018-07-08T00:43:05Z

That's because it passed 😉 I'll add a message to say it's okay on there.

cclauss · 2018-07-08T05:24:48Z

Fixes #11550

tacaswell · 2018-07-08T15:21:45Z

.travis.yml

@@ -160,6 +160,9 @@ before_script: |
 script: |
  echo "Calling pytest with the following arguments: $PYTEST_ADDOPTS"
  python -mpytest
+  if [[ $RUN_FLAKE8 == 1 ]]; then


I suspect this is going to have the same issue we discovered in the pytzectomy PR where if the tests fail but flake8 passes, travis will report success.

Probably set -e is the way to go?
Or add || return 1 at the end of each (relevant) line.

I don't like set -e; it bleeds into Travis' code and causes spurious errors, so you need to remember to turn it off. The last time that happened, we ended up just removing it instead. I split these into separate entries instead, which does fail correctly.

tacaswell · 2018-07-08T15:25:19Z

examples/units/basic_units.py

@@ -25,13 +25,13 @@ def __get__(self, obj, objtype=None):


 class TaggedValueMeta(type):
-    def __init__(cls, name, bases, dict):


This is used as a meta-class so I ma not sure that this change is correct (I am also not sure off the top of my head what self is in this case and when this __init__ gets called).

It may be that the object is a class/type here, but it's still an instance at this point, so I'm not sure calling it something other than self is warranted.

tacaswell

I think that the way this is called in travis will mask failures in the test suite from failing the test.

Anyone can clear this if this is fixed or they are sure I am wrong.

cclauss · 2018-07-08T16:41:47Z

Do the flake8 tests first and the pytests after?

jklymak · 2018-07-08T16:44:26Z

@cclauss then it won't fail if flake8 fails?

#11597 shows that @tacaswell is indeed correct - that failing a test still returns a positive Travis check the way this is setup now....

Doesn't catch failing pytest tests anymore

jklymak · 2018-07-08T17:07:53Z

#11597 shows that adding set -e causes a failure when pytest should fail. Testing if it fails if there is a flake8 error

QuLogic · 2018-07-08T17:47:50Z

Splitting the two commands into separate entries should work fine.

fixed.

tacaswell · 2018-07-08T18:37:42Z

Thanks everyone!

QuLogic added the topic: testing label Jun 22, 2018

QuLogic added this to the v3.0 milestone Jun 22, 2018

QuLogic mentioned this pull request Jun 22, 2018

Run flake8 instead of pep8 on Python 3.6 #10940

Closed

6 tasks

jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jun 22, 2018

QuLogic mentioned this pull request Jun 22, 2018

Doc: beautify usetex demo example #11462

Merged

6 tasks

cclauss approved these changes Jun 22, 2018

View reviewed changes

anntzer reviewed Jun 22, 2018

View reviewed changes

QuLogic force-pushed the flake8 branch from 8535090 to 306ff1d Compare June 22, 2018 07:18

anntzer added the status: needs rebase label Jun 23, 2018

timhoffm approved these changes Jun 23, 2018

View reviewed changes

QuLogic force-pushed the flake8 branch from 306ff1d to dfa2c2c Compare June 23, 2018 19:07

QuLogic removed the status: needs rebase label Jun 23, 2018

dstansby mentioned this pull request Jun 29, 2018

Re-enable pep8 on examples folder #11538

Merged

QuLogic force-pushed the flake8 branch from dfa2c2c to d4907dc Compare June 29, 2018 21:10

anntzer mentioned this pull request Jul 2, 2018

Undefined names: 'obj_type' and 'cbook' #11550

Closed

QuLogic force-pushed the flake8 branch from d4907dc to 3e4c194 Compare July 7, 2018 08:47

cclauss suggested changes Jul 7, 2018

View reviewed changes

QuLogic force-pushed the flake8 branch from 3e4c194 to b36dfe2 Compare July 7, 2018 20:51

QuLogic added 3 commits July 7, 2018 19:27

Use raw strings to avoid invalid escape sequences.

34d8eb2

STY: Use consistent (class)method parameter names.

99a6b8a

STY: Fix N813 camelcase imported as lowercase.

d09bfdf

QuLogic force-pushed the flake8 branch 2 times, most recently from 9d55403 to 6fa1391 Compare July 7, 2018 23:57

QuLogic changed the title ~~Enable flake8 and re-enable pep8 on examples~~ Enable flake8 and re-enable it everywhere Jul 7, 2018

QuLogic force-pushed the flake8 branch from 6fa1391 to bf1c77c Compare July 8, 2018 00:44

jklymak previously approved these changes Jul 8, 2018

View reviewed changes

cclauss approved these changes Jul 8, 2018

View reviewed changes

QuLogic mentioned this pull request Jul 8, 2018

TST: testing not catching bad escape sequences in doc strings #11335

Closed

tacaswell reviewed Jul 8, 2018

View reviewed changes

tacaswell previously requested changes Jul 8, 2018

View reviewed changes

jklymak mentioned this pull request Jul 8, 2018

TST: Don't review/merge Test travis #11597

Closed

6 tasks

TST: Run flake8 outside of pytest.

6123369

QuLogic force-pushed the flake8 branch from bf1c77c to 6123369 Compare July 8, 2018 17:44

tacaswell merged commit 94f41d1 into matplotlib:master Jul 8, 2018

QuLogic deleted the flake8 branch July 8, 2018 20:41

QuLogic mentioned this pull request Jul 16, 2018

Fix invalid escape sequences on branch v2.2.x #11669

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable flake8 and re-enable it everywhere #11477

Enable flake8 and re-enable it everywhere #11477

QuLogic commented Jun 22, 2018

QuLogic commented Jun 22, 2018

anntzer Jun 22, 2018

QuLogic Jun 22, 2018

tacaswell Jul 7, 2018

cclauss commented Jun 22, 2018

QuLogic commented Jun 23, 2018

timhoffm commented Jun 23, 2018

anntzer commented Jul 2, 2018

jklymak commented Jul 6, 2018

cclauss commented Jul 6, 2018

QuLogic commented Jul 7, 2018

cclauss commented Jul 7, 2018 •

edited

Loading

tacaswell commented Jul 7, 2018

tacaswell commented Jul 7, 2018

cclauss commented Jul 7, 2018 •

edited

Loading

cclauss left a comment

QuLogic commented Jul 7, 2018

jklymak commented Jul 8, 2018

QuLogic commented Jul 8, 2018

cclauss commented Jul 8, 2018

tacaswell Jul 8, 2018

anntzer Jul 8, 2018

QuLogic Jul 8, 2018

tacaswell Jul 8, 2018

QuLogic Jul 8, 2018

tacaswell left a comment

cclauss commented Jul 8, 2018

jklymak commented Jul 8, 2018

jklymak commented Jul 8, 2018

QuLogic commented Jul 8, 2018

tacaswell commented Jul 8, 2018

		@@ -25,13 +25,13 @@ def __get__(self, obj, objtype=None):


		class TaggedValueMeta(type):
		def __init__(cls, name, bases, dict):

Enable flake8 and re-enable it everywhere #11477

Enable flake8 and re-enable it everywhere #11477

Conversation

QuLogic commented Jun 22, 2018

PR Summary

PR Checklist

QuLogic commented Jun 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cclauss commented Jun 22, 2018

QuLogic commented Jun 23, 2018

timhoffm commented Jun 23, 2018

anntzer commented Jul 2, 2018

jklymak commented Jul 6, 2018

cclauss commented Jul 6, 2018

QuLogic commented Jul 7, 2018

cclauss commented Jul 7, 2018 • edited Loading

tacaswell commented Jul 7, 2018

tacaswell commented Jul 7, 2018

cclauss commented Jul 7, 2018 • edited Loading

cclauss left a comment

Choose a reason for hiding this comment

QuLogic commented Jul 7, 2018

jklymak commented Jul 8, 2018

QuLogic commented Jul 8, 2018

cclauss commented Jul 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell left a comment

Choose a reason for hiding this comment

cclauss commented Jul 8, 2018

jklymak commented Jul 8, 2018

jklymak commented Jul 8, 2018

QuLogic commented Jul 8, 2018

tacaswell commented Jul 8, 2018

cclauss commented Jul 7, 2018 •

edited

Loading

cclauss commented Jul 7, 2018 •

edited

Loading