Repository navigation
PEP8: making travis run pep8 on diff instead of comparing the total number of violation to be more robust. #7358
Description
The objective of this issue is to move the discussion of PEP8 away from PR #7344, and also bring some liberty to contributors that may be affected by what is described below.
Background
The problem is PEP8 (pre-August 2012) was not clear about the spacing around arithmetic operations. The lack of clarity lead to the impression that it was recommended to have a space around all arithmetic operators. This problem was discussed here and the fixed in August 2012. Accordingly, the pep8 the tool became agnostic to no whitespace around arithmetic operators, (PyCQA/pycodestyle#123) -- it allowed for user judgement.
Of-course, through all this many had already been conditioned to write compliant code with adherence to the rigid spacing suggestions of pre-August 2012 PEP8.
Issue
The codebase is full of expressions that largely adhere to the pre-August 2012 PEP8, and many of those expressions are not as clear as they could be. Now, re-writing them for readability is debatable as it would create "git blame" noise. Also, the uniformity of the existing expressions in the codebase cannot serve as a reason for incoming code to adhere a similar form.
The actionable issue raised by @NelleV is that the PEP8 style checks only look for increases in violations.
Solution
Suggested by @NelleV, run flake8 on the diff of the patch. Projects doing this are:
- https://github.com/joblib/joblib - 9 months
- https://github.com/scikit-learn/scikit-learn - 2 months
Activity
NelleV commentedon Oct 29, 2016
Here is the PR adding this to travis on sklearn: scikit-learn/scikit-learn#7127
(and ping @Carreau, cause we talked about this for ipython)
Carreau commentedon Oct 29, 2016
Thanks for pinging me ! One things I'd also like to figure out is how to have it do that locally only my non-committed changes in my editor :-)
has2k1 commentedon Oct 29, 2016
@Carreau, indeed. The noise from existing violations makes a
pre-commithook the easier solution. But we want more.NelleV commentedon Oct 29, 2016
@Carreau Remind me which editor you use?
[-]PEP8[/-][+]PEP8: [/+][-]PEP8: [/-][+]PEP8: making travis run pep8 on diff instead of comparing the total number of violation to be more robust.[/+]QuLogic commentedon Oct 29, 2016
One hopes that by the time 2.0 is out, we can get rid of most of these on
master...lesteve commentedon Nov 7, 2016
Great to see that there is some interest for this tool outside joblib and scikit-learn ! Full disclosure, it is not perfect. For example, one slight issue I discovered recently is that adding a new line on top of the standard two lines between two functions won't result in a failure. The reason for this is that we do a diff without a context to avoid getting failures for PEP8 violations that already exist. Without seeing the context you can not tell that the additional new line is a PEP8 violation.
It has been quite useful though because people tend to fix PEP8 violations on their own looking at the Travis status and it does free maintainers some time to focus on more important things.
flake8_diff.sh also changes from time to time and I admit to keeping it in sync completely manually between joblib and scikit-learn. Maybe we should think about a way to share it between different projects. Suggestions more than welcome!
NelleV commentedon Nov 8, 2016
@lesteve We have a current system for testing pep8 that is incorporated in the tests which doesn't have this particular problem (but has many others, including being extremely fragile). I wonder if we could have best of both world by combining our experience.
anntzer commentedon Jan 9, 2017
Something like https://flake8-diff.readthedocs.io/en/latest/index.html could be helpful? (Not sure, but it looks like a wrapper around
flake8 --diffwith explicit VCS support.)Carreau commentedon Jan 9, 2017
There is also pep8radius, that fix pep8 on the diff. So you don't have to fix it manually.
I'm working on a bot you can mention to apply the fix and push a change on the PR. The only drawback I have for now is that the GitHub "Api" does not respect the "Allow edit from maintainers" flag, so you have to ask the user to activate the bot on their account for it to work. Which is less than optimal.
The alternative is to host a copy of the bot yourself and give it a personal access token, which is less than ideal if it is possible to get one for all python projects on GitHub which is a simple to activate as Travis.
lesteve commentedon Jan 10, 2017
My 2 cents from the scikit-learn side: flake8_diff.sh is very useful. Even if it has a few gotchas, it does help reviewers to focus on more important issues than flake8 problems. Also, PR authors, even the less technical ones, manage to fix flake8 errors by themselves in order to get Travis green. Maybe it does help that the Travis build error mention the line and the line numbers as they are on the PR author machine (i.e. we use the PR branch and not the result of the PR branch into master, Travis does the latter by default).
Of course I am interested by better approaches so let me know if you come up with a better solution!
NelleV commentedon Jan 13, 2017
From working on different projects, I agree that @lesteve is the more robust and flexible approach. The only limitation to it is the fact that it works locally and not globally. But it is robust, fast, maintainable and doesn't return failures when it shouldn't like our current system.
As expressed to @Carreau in person yesterday, I am quite concern about automatically attempts to fix pep8 compliancy automatically: I've had mixed results with those and have always resorted to fixing them by hand (even on 8k loc files). Considering it is not an automatic tool, but has to be manually activated, I am fine with testing it on different projects. (Interestingly, @Carreau's approach could automatically break our tests quite frequently :p )
@Carreau A thought about the bot: instead of pushing to the branch, could it create a PR against the original branch?
Carreau commentedon Jan 13, 2017
Yes I believe it could. Shouldn't be too hard.
One other extension possibility would be for the bot to comment on/about the non-pep-8 compliant lines with a checkable markdown list and ask you to check which lines/violation you want to autofix, or ask you to mention it on specific lines when using the "review" workflow.
That's more complicated though.
anntzer commentedon Oct 2, 2017
I think this has been obsoleted by the switch to pytest-pep8.