Switch to pytest-pep8. #8001
Switch to pytest-pep8. #8001
Conversation
|
Rebased due to conflicts. Note, this PR is independent of the other pytest ones, and should be working at this point. |
| - python: 3.6 | ||
| env: USE_PYTEST=true DELETE_FONT_CACHE=1 TEST_ARGS= | ||
| env: USE_PYTEST=true DELETE_FONT_CACHE=1 INSTALL_PEP8=pytest-pep8 RUN_PEP8=--pep8 |
There was a problem hiding this comment.
Can you run pep8 without installing it? The two options seem redundant.
There was a problem hiding this comment.
I think the one option handles optionally installing it on builds that use it, the other handles adding the argument to running pytest.
There was a problem hiding this comment.
It be easier to have a boolean flag IMO
There was a problem hiding this comment.
Then you have to have bash code that checks the boolean and manually install. I like this style better because the environment variables are always defined--less branching. It actually simplifies .travis.yml.
| @@ -1,2 +1,134 @@ | |||
| [pytest] | |||
| norecursedirs = .git build ci dist extern release tools unit venv | |||
| 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 | |||
There was a problem hiding this comment.
If I understand this file properly, we are just plainly ignoring a bunch of errors for each file?
I don't like our old system very much, but how is that going to be useful?
There was a problem hiding this comment.
We could enable them again one-by-one? Maybe? shrugs
Could replace * with the files we don't expect to pass.
There was a problem hiding this comment.
This line ignores errors based on our existing config + pep8's default ignore list. I tried not ignoring these globally, but they apply to a lot of files, so it's very difficult to limit this to just the files that need it.
The files we don't expect to pass are already listed below with the specific ignores.
There was a problem hiding this comment.
E121,E123,E126,E226,E24,E704 are the defaults that come from pep8.
There was a problem hiding this comment.
I see now--that's what I get for a quick reply.
There was a problem hiding this comment.
Wait, I take some of that back. Some of these ignores apply to many files, but some apply to no files at all. I will remove the redundant ones.
There was a problem hiding this comment.
Presumably it would be really tedious but not too hard to slowly fix each PEP8 violation for each file and remove all the lines below one at a time.
There was a problem hiding this comment.
This will create huge amount of conflicts with the rest of the pull requests.
This is generally independent, so I opened it as a separate PR. It puts us one step closer to removing the top-level
conftest.pywhich will fix brokenness with #7974 on AppVeyor.I chose to be explicit about which pep8 errors are being skipped so that we don't accidentally introduce even more issues.