Pep8 on the axes module #1417

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

NelleV commented Oct 20, 2012

Reopening this PR with rebased code.

Thanks,
N

Member

dmcdougall commented Nov 7, 2012

Currently, there's no way for me to provide inline comments as feedback on this pull request. GitHub is hiding the diff because it is big. I have emailed support to try and get this resolved. It is pretty crucial that other developers and contributors are able to provide inline feedback on PEP8 pull requests. I will touch base again when GitHub support reaches out to me.

Contributor

NelleV commented Nov 7, 2012

I can try to break this into different PR if github doesn't react "quickly".

Owner

efiring commented Nov 7, 2012

On 2012/11/07 7:19 AM, Damon McDougall wrote:

Currently, there's no way for me to provide inline comments as feedback
on this pull request. GitHub is hiding the diff because it is big. I
have emailed support to try and get this resolved. It is pretty crucial
that other developers and contributors are able to provide inline
feedback on PEP8 pull requests. I will touch base again when GitHub
support reaches out to me.


Reply to this email directly or view it on GitHub
#1417 (comment).

Maybe this is a sign that the PEP8 changes to axes.py should be done in
stages, not all at once. Say, perhaps roughly 1/4 of the lines at a time.

Also, I think @WeatherGod was doing, or at least proposed to do, some
major much-needed refactoring to slim down axes.py by breaking related
chunks of functionality out into their own modules. I don't know how
far along he is, but if it is substantial, then it would make sense to
not do a PEP8 makeover until after he has done his refactoring.

Eric

Member

WeatherGod commented Nov 7, 2012

On Wed, Nov 7, 2012 at 1:01 PM, Eric Firing notifications@github.comwrote:

Maybe this is a sign that the PEP8 changes to axes.py should be done in
stages, not all at once. Say, perhaps roughly 1/4 of the lines at a time.

+1

Also, I think @WeatherGod was doing, or at least proposed to do, some
major much-needed refactoring to slim down axes.py by breaking related
chunks of functionality out into their own modules. I don't know how
far along he is, but if it is substantial, then it would make sense to
not do a PEP8 makeover until after he has done his refactoring.

Not very far along, and most of the initial changes were dealing with PEP8
and deprecated syntax anyway. It would be best to start fresh here.

Member

dmcdougall commented Nov 7, 2012

From Ben Lavendar at GitHub support:

Damon,

Sorry, but we max out diffs at 100k per file. This diff is 224k.

You'll need to break it into smaller commits if you want to do line-by-line commenting.
Sorry about that.
Member

ivanov commented Nov 8, 2012

so I know I'm really late to the party, but to me this is a pretty big sign that we shouldn't even be doing this in the first place. In most of the changes, I don't see a clear benefit. For example: removing the trailing \ on line continuations. I think style changes are fine as they are incorporated in actual changes to the code, but having purely stylistic changes all through the code base (and in tons of separate commits throughout the codebase) is the wrong way to go. (edited, see my next comment below)

There was one big commit to IPython that went through and removed a whole bunch of trailing whitespace. I have now bumped into the commit several times when trying to figure out when particular lines of code were last changed, except that it wasn't a functional or informative changeset, just an annoyance to hope over and makes git blame much less effective at trying to track down bugs. (Now, for that particular case, it turns out there's git blame -w which ignores whitespace, but there won't be such a friendly "ignore PEP8" changes flag.

Contributor

NelleV commented Nov 8, 2012

I'll wait for @WeatherGod to finish the refactoring: this file really needs one, and the pep8 changes will be much much easier when splitted. If you need help with it, give me a shout.

@ivanov I'm tired and I've been working quite hard on those patches so I'm reacting quite quite strongly here:

First, I've been working on this for more than 2 months now, so indeed, your concern comes a bit late, specially as I asked whether those changes were welcome, how to proceed...

Why do I pep8 changes? Like many developpers, I don't work on non pep8 compliant code because non pep8 compliant distract me from the real problems. More and more people in the scientific community will not contribute to a project which is not pep8 (one of IPython's core dev once even told me he refused to review non pep8 compliant code). I initially started looking at the codebase to fix a bug. It turned out that the tests didn't run on python2.6: the reason was a typo in the code, something that is easily spotted when running flake8 on the code (which I automatically do, each time I save a file in vim). That's what pushed me to go forward with the patches.

Why should matplotlib be pep8-compliant?

  1. Make the code more readable
  2. Code style conventions allows to smoothen style differences between developpers, hence allowing everyone to get in the code faster (who has not reindented a C++ file completely once in his life?).
  3. Those changes allowed us to spot not working, untested code and fix those code.
  4. Increase the number of contributors by lowering the entry cost.
  5. Last but not least, the problem in the patch on the axes file is not that we are doing pep8 changes, but that the file is insanely big (10kloc) and needs to be refactored. This patch just underlines this.

I can continue the list, but I don't see the point in that. If you feel strongly that we should not make those pep8 changes, I think this discussion should happen on the mailing list, not on github, so that everyone can be aware of it and chime in.

Member

ivanov commented Nov 8, 2012

@NelleV Thank you for your contributions (and all the future ones, too 👍 !). The last thing I want to do is discourage anyone from participating. We all volunteer our time and effort and I did not intend to take anything away from the work you've been doing.

I really mean "party" quite literally in "being late to the party" - development in these projects is always a rush of excitement and fun for me, and I hate it when anyone or anything derails that - so I apologize if I caused those kinds of feelings.

I have no arguments against PEP8 compliance, and do feel strongly that mpl should converge toward PEP8. PEP8 is great! (I use kevinw/pyflakes-vim and vim-scripts/pep8.git, btw - go team vim!) When I wrote "this is a pretty big sign that we shouldn't even be doing this in the first place" - the "this" was referring to "this big of a change to the codebase" I was simply trying to convey the sentiment that @efiring also expressed in [mpl-devel] strategy for 1.2.x, master, PEP8 changes:

We have a flood of PEP8 PRs based on master, many of which have been merged, some by myself--so I have no objection to this aspect of the situation, though I would have preferred a slower pace, a garden hose rather than a fire hose.

I added the emphasis. I also wrote

I think style changes are fine as they are incorporated in actual changes to the code, but having purely stylistic changes all through the code base (and in tons of separate commits throughout the codebase) is the wrong way to go.

but here, I was actually trying to express my preference for doing PEP8 changes, and reading back through the mailing list archives, it seems no one else brought up this concern, so I'm fine with this going full steam ahead! And I'm tired too, and what I wrote there doesn't even make sense since it's inconsistent with me being frightened by having to drink from a fire hose - separate commits on separate files (the way you've been doing them) actually is the way to go with making such changes. There'd be no way in hell to review one incredibly long diff that just makes all PEP8 fixes in the whole codebase -- so I'm going to go back and cross out the rest of that sentence in the original.

Member

WeatherGod commented Nov 8, 2012

Just as another vote of confidence, I am also very supportive of the PEP8
changes. If anything, the question is "when?" and "how much?" not "should
we?". But then, you start to easily slip into the "I'll do it later" mode,
which is bad. Better to treat this like a band-aid (or a couple of
band-aids...)

Don't wait for me with my refactor. I simply do not have the time and the
resources to do it anytime soon. Plus, I have to first do a MEP and have
very lengthy discussions with it, and the implementation will likely happen
in stages anyway. I would rather work on a clean slate.

Contributor

NelleV commented Nov 8, 2012

@WeatherGod: I'll proceed in splitting this PR into smaller chunks so that it can be reviewed then.

I'm closing this PR.

NelleV closed this Nov 8, 2012

Member

pelson commented Nov 8, 2012

I'll proceed in splitting this PR into smaller chunks so that it can be reviewed then.

That would be great, thanks @NelleV! I've just been PEP8-ing a code base for another project and I know how tedious doing this can be - it is very much appreciated!

@ivanov : I can see the problem you are describing on the web interface, and agree that is certainly a downside to doing stylistic changes in one go, but can't figure out how the stylistic changes (which don't break anything) can impact git bisect.

EDIT: I missread your post @ivanov - It doesn't effect git bisect it effects git blame. I've never used git blame... please ignore me! 😄

Member

dmcdougall commented Nov 8, 2012

@pelson I have used git blame. It's actually really useful if you need to follow up on a line change.

Contributor

NelleV commented Dec 9, 2012

@WeatherGod I've finished the pep8 fixes on the axis module. I've got another PR to remove some code, but I think you can start refactoring. I had a look at how to do that, and a small refactoring based on classes seem easy to do. The rest is going to be much harder... (I managed to create a submodule axes, with 2 files, but one of those is still 8kloc long)

Contributor

NelleV commented Jan 7, 2013

@WeatherGod I was wondering whether you started on the refactoring of the axes module: if not, I can work on this.

Cheers,
N

Member

WeatherGod commented Jan 7, 2013

Go right ahead. Don't wait for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment