Hi All, I've been doing some PEP8 work using autopep8. One problem that has turned up is that the default behavior of autopep8 is version dependent. I'd like to put a script in numpy tools that runs autopep8 with some features disabled, namely 1. E226 -- puts spaces around arithmetic operators (+, -, *, /, **). 2. E241 -- allows only single spaces after ',' The first leaves expression formatting in the hands of the coder and avoids things like "2 ** 3". The second allows array entries to be vertically aligned, which can be useful in clarifying the values used in tests. A few other things that might need decisions: 1. [:,:, 2] or [:, :, 2] 2. Blank line before first function after class Foo(): The advantage of having a fixed script for PEP8 compliance is that once the code is brought into compliance it will stay there and we can recommend running the script before pushing. That will not only keep the code looking neat and regular, but will also strip trailing whitespace. Of course, the autopep8 folks might make further changes in the defaults that will cause trouble in the future, but I hope that will be a minor nuisance that we can deal with. There are options for autopep8<https://pypi.python.org/pypi/autopep8>and pep8 <https://pep8.readthedocs.org/en/latest/intro.html#error-codes> in case folks have other suggestions for features to add or disable. Thoughts? Chuck
Hi All,
I've been doing some PEP8 work using autopep8. One problem that has turned up is that the default behavior of autopep8 is version dependent. I'd like to put a script in numpy tools that runs autopep8 with some features disabled, namely
1. E226 -- puts spaces around arithmetic operators (+, -, *, /, **). 2. E241 -- allows only single spaces after ','
Something we have done in matplotlib is that we have made PEP8 a part of
things like "2 ** 3". The second allows array entries to be vertically aligned, which can be useful in clarifying the values used in tests. A few other things that might need decisions:
1. [:,:, 2] or [:, :, 2] 2. Blank line before first function after class Foo():
For the first one, I prefer spaces. For the second one, I prefer no blank
On Sat, Sep 7, 2013 at 7:56 PM, Charles R Harris <charlesr.harris@gmail.com>wrote: the tests. We are transitioning, but the idea is that eventually, with Travis, all pull requests will get PEP8 checked. I am very leary of automatic PEP8-ing. I would rather have the tests fail and let me manually fix it rather than have code automatically changed. The first leaves expression formatting in the hands of the coder and avoids lines. Cheers! Ben Root
Something we have done in matplotlib is that we have made PEP8 a part of the tests.
In Iris and Cartopy we've also done this and it works well. While we transition we have an exclusion list (which is gradually getting shorter). We've had mixed experiences with automatic reformatting, so prefer to keep the human in the loop. Richard
On Mon, Sep 9, 2013 at 8:12 AM, Richard Hattersley <rhattersley@gmail.com>wrote:
Something we have done in matplotlib is that we have made PEP8 a part of the tests.
In Iris and Cartopy we've also done this and it works well. While we transition we have an exclusion list (which is gradually getting shorter). We've had mixed experiences with automatic reformatting, so prefer to keep the human in the loop.
I agree with keeping a human in the loop, the script would be intended to get things into the right neighborhood, the submitter would have to review the changes after. If the script isn't too strict there will be than one way to do some things and those bits would rely on the good taste of the coder. Chuck
On Mon, Sep 9, 2013 at 3:29 PM, Charles R Harris <charlesr.harris@gmail.com> wrote:
On Mon, Sep 9, 2013 at 8:12 AM, Richard Hattersley <rhattersley@gmail.com> wrote:
Something we have done in matplotlib is that we have made PEP8 a part of the tests.
In Iris and Cartopy we've also done this and it works well. While we transition we have an exclusion list (which is gradually getting shorter). We've had mixed experiences with automatic reformatting, so prefer to keep the human in the loop.
I agree with keeping a human in the loop, the script would be intended to get things into the right neighborhood, the submitter would have to review the changes after. If the script isn't too strict there will be than one way to do some things and those bits would rely on the good taste of the coder.
So if I understand right, the goal is to have some script that developers can run before (or after) submitting a PR, like tools/autopep8-my-changes numpy/ that will fix up their changes, but leave the rest of numpy alone? And the proposed mechanism is to come up with a combination of changes to the numpy source and an autopep8 configuration such that autopep8 --our-config numpy/ becomes a no-op, and then we can use this as an implementation of tools/autopep8-my-changes? If that's right then my feeling is that the goal seems worthwhile but the approach seems difficult and unlikely to survive for long. As soon as someone overrides autopep8 once, we either have to disable the rule for the whole project or keep overriding it manually forever. You're already suggesting taking out the spaces-around-arithmetic rule, which strikes me as one of the most useful -- sure, it gets things wrongs sometimes, but I feel like we're constantly reviewing PRs where all*the*(arithmetic+is)-written**like*this. Maybe a better approach would be to spend that time hacking up some script that uses git and autopep8 together to run autopep8 over all and only those lines which the current branch has actually touched? It's pretty easy to parse 'git diff' output to get a list of all line numbers which have been modified, and then we could run autopep8 over the modified files and pull out only those changes which touch those lines. -n P.S.: definitely [:, :, 2]
I think a good solution would to use add a git_hooks directory with a pre-commit git hook along with an git hook installation script. And a note should be added to DEV_README.txt suggesting installing the git hooks for pep8 compatibility. I personally use this as a pre-commit #!/bin/sh FILES=$(git diff --cached --name-status | grep -v ^D | awk '$1 $2 { print $2}' | grep -e .py$) if [ -n "$FILES" ]; then pep8 -r $FILES fi which is from here: https://gist.github.com/lentil/810399#comment-303703 On Mon, Sep 9, 2013 at 10:54 AM, Nathaniel Smith <njs@pobox.com> wrote:
On Mon, Sep 9, 2013 at 3:29 PM, Charles R Harris <charlesr.harris@gmail.com> wrote:
On Mon, Sep 9, 2013 at 8:12 AM, Richard Hattersley <
rhattersley@gmail.com>
wrote:
Something we have done in matplotlib is that we have made PEP8 a part
of
the tests.
In Iris and Cartopy we've also done this and it works well. While we transition we have an exclusion list (which is gradually getting shorter). We've had mixed experiences with automatic reformatting, so prefer to keep the human in the loop.
I agree with keeping a human in the loop, the script would be intended to get things into the right neighborhood, the submitter would have to review the changes after. If the script isn't too strict there will be than one way to do some things and those bits would rely on the good taste of the coder.
So if I understand right, the goal is to have some script that developers can run before (or after) submitting a PR, like tools/autopep8-my-changes numpy/ that will fix up their changes, but leave the rest of numpy alone?
And the proposed mechanism is to come up with a combination of changes to the numpy source and an autopep8 configuration such that autopep8 --our-config numpy/ becomes a no-op, and then we can use this as an implementation of tools/autopep8-my-changes?
If that's right then my feeling is that the goal seems worthwhile but the approach seems difficult and unlikely to survive for long. As soon as someone overrides autopep8 once, we either have to disable the rule for the whole project or keep overriding it manually forever. You're already suggesting taking out the spaces-around-arithmetic rule, which strikes me as one of the most useful -- sure, it gets things wrongs sometimes, but I feel like we're constantly reviewing PRs where all*the*(arithmetic+is)-written**like*this.
Maybe a better approach would be to spend that time hacking up some script that uses git and autopep8 together to run autopep8 over all and only those lines which the current branch has actually touched? It's pretty easy to parse 'git diff' output to get a list of all line numbers which have been modified, and then we could run autopep8 over the modified files and pull out only those changes which touch those lines.
-n
P.S.: definitely [:, :, 2] _______________________________________________ NumPy-Discussion mailing list NumPy-Discussion@scipy.org http://mail.scipy.org/mailman/listinfo/numpy-discussion
I made a PR request about supplying a git hooks framework at https://github.com/numpy/numpy/pull/200. I asked for it to be closed because I couldn't easily figure our how to handle x-platform issues. If you have an answer, what I was working on might be a start. But your script is an example of the x-platform challenges since it will only run on Windows that has a Linux environment installed (Mingw or the like). What I have started to use in my projects is 'tox', 'coverage', and 'flake8'. Really nice stuff. There is an issue though with the 'flake8' report. There are only very localized places in a few files that I want to ignore some portion or other of PEP8, so I don' t insist that 'flake8' pass, I just ignore the output where I need to ignore it. Usually what I want to ignore is the spaces around brackets and commas in order to line up array values as mentioned in an earlier post within this thread. Here is an example of my project setup based in part on 'cookiecutter' - https://bitbucket.org/timcera/astronomia/src. Yes, I use bitbucket - so sue me. Magic happens inside the 'tox.ini' file. I suspect that for numpy, making something like 'flake8' part of the tests would work better than 'autopep8'. 'autopep8' can be configured to just report with the --list-fixes option, which would give people a little more confidence to use it rather that it's primary mission as an editor. Another plug for 'flake8'; it can be configured on a file by file basis to ignore particular errors or warnings, and exits with an error status if the file doesn't pass. I do think the idea has merit, but this entire process would be a lot of work and I am not stepping forward to do it. At this point I simply have to say that I am a 'balcony muppet'. Much thanks to Josef for the reminder about where I learned my curmudgeony ways. Those guys made the show! Kindest regards, Tim On Mon, Sep 9, 2013 at 12:08 PM, Blake Griffith <blake.a.griffith@gmail.com> wrote:
I think a good solution would to use add a git_hooks directory with a pre-commit git hook along with an git hook installation script. And a note should be added to DEV_README.txt suggesting installing the git hooks for pep8 compatibility. I personally use this as a pre-commit
#!/bin/sh
FILES=$(git diff --cached --name-status | grep -v ^D | awk '$1 $2 { print $2}' | grep -e .py$) if [ -n "$FILES" ]; then pep8 -r $FILES fi
which is from here: https://gist.github.com/lentil/810399#comment-303703
On Mon, Sep 9, 2013 at 10:54 AM, Nathaniel Smith <njs@pobox.com> wrote:
On Mon, Sep 9, 2013 at 3:29 PM, Charles R Harris <charlesr.harris@gmail.com> wrote:
On Mon, Sep 9, 2013 at 8:12 AM, Richard Hattersley <rhattersley@gmail.com> wrote:
Something we have done in matplotlib is that we have made PEP8 a part of the tests.
In Iris and Cartopy we've also done this and it works well. While we transition we have an exclusion list (which is gradually getting shorter). We've had mixed experiences with automatic reformatting, so prefer to keep the human in the loop.
I agree with keeping a human in the loop, the script would be intended to get things into the right neighborhood, the submitter would have to review the changes after. If the script isn't too strict there will be than one way to do some things and those bits would rely on the good taste of the coder.
So if I understand right, the goal is to have some script that developers can run before (or after) submitting a PR, like tools/autopep8-my-changes numpy/ that will fix up their changes, but leave the rest of numpy alone?
And the proposed mechanism is to come up with a combination of changes to the numpy source and an autopep8 configuration such that autopep8 --our-config numpy/ becomes a no-op, and then we can use this as an implementation of tools/autopep8-my-changes?
If that's right then my feeling is that the goal seems worthwhile but the approach seems difficult and unlikely to survive for long. As soon as someone overrides autopep8 once, we either have to disable the rule for the whole project or keep overriding it manually forever. You're already suggesting taking out the spaces-around-arithmetic rule, which strikes me as one of the most useful -- sure, it gets things wrongs sometimes, but I feel like we're constantly reviewing PRs where all*the*(arithmetic+is)-written**like*this.
Maybe a better approach would be to spend that time hacking up some script that uses git and autopep8 together to run autopep8 over all and only those lines which the current branch has actually touched? It's pretty easy to parse 'git diff' output to get a list of all line numbers which have been modified, and then we could run autopep8 over the modified files and pull out only those changes which touch those lines.
-n
P.S.: definitely [:, :, 2] _______________________________________________ NumPy-Discussion mailing list NumPy-Discussion@scipy.org http://mail.scipy.org/mailman/listinfo/numpy-discussion
_______________________________________________ NumPy-Discussion mailing list NumPy-Discussion@scipy.org http://mail.scipy.org/mailman/listinfo/numpy-discussion
participants (6)
-
Benjamin Root -
Blake Griffith -
Cera, Tim -
Charles R Harris -
Nathaniel Smith -
Richard Hattersley