Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Switch to pytest-pep8.#8001
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
Uh oh!
There was an error while loading.Please reload this page.
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=1TEST_ARGS= | ||
| env: USE_PYTEST=true DELETE_FONT_CACHE=1INSTALL_PEP8=pytest-pep8 RUN_PEP8=--pep8 |
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.
Can you run pep8 without installing it? The two options seem redundant.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It be easier to have a boolean flag IMO
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This line ignores errors based on ourexisting config +pep8's default ignore list. I tried not ignoring these globally, but they apply to alot 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
E121,E123,E126,E226,E24,E704 are the defaults that come frompep8.
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 see now--that's what I get for a quick reply.
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.
Wait, I take some of that back. Some of these ignores apply to many files, butsome apply to no files at all. I will remove the redundant ones.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This will create huge amount of conflicts with the rest of the pull requests.
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.
👍
Uh oh!
There was an error while loading.Please reload this page.
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.