PEP 8-ify the codebase

[melinath]

Right now, we use tabs and long lines everywhere. This makes our code harder to read. I'd like to switch to 80-character lines and 4-space indentation. (As well as potentially other PEP 8 changes.) Does that sound all right?

[harrislapiroff]

If we must. I loathe 4-space indentation and 80 character lines, tbh, but I respect that we should probably play nice with what everyone else does.

[philipkimmey]

I'm on it!

[jspiros]

This is ridiculous. PEP8 clearly states that you simply use spaces or tabs consistently within a given project, and this project consistently uses tabs currently. Changing nearly every line of code is a waste of time, and at minimum will ruin our ability to use git blame.

[harrislapiroff]

@jspiros makes a compelling point...

[melinath]

@jspiros does not make a compelling point IMO. It brings us in line with the majority of python projects, and the effect on git blame is a minor inconvenience, not an earth-shattering issue.

[jspiros]

@melinath What is the real world benefit of being "in line with the majority of python projects"? It doesn't change the fact that there are still a significant minority of python projects that use tabs, not spaces, so it doesn't stop any given Python developer from needing to pay attention and configure their editor accordingly on a per-project basis anyway.

I would argue that "being in line with the majority of python projects" is a minor convenience for some at best, not an earth-shattering issue, in that it allows some developers to avoid a few minutes of configuration when they start working on the project. But even that depends on what projects a given developer has worked on and currently works on. All of Oberlin's projects, internal or external, use PEP8 with the exception of these two rules — we use tabs instead of spaces, we don't hard wrap lines, and we don't format whitespace beyond what is necessary — and for most of us, these projects are the vast majority of python projects that we work on. I would guess that, as far as Oberlin developers are concerned, our default editor settings favor the status quo. So, for that group of developers, by your own standards, the change itself would be inconvenient, besides the git blame issue.

I think that developers in general should be respectful of the style rules in effect for the projects that they work on, and should be flexible given that, with or without django-daguerre, there are many different style rules in effect throughout the Python ecosystem. Given this, I don't think the style rules currently in effect cause a significant barrier to entry for new developers wanting to work on django-daguerre (though I do think a note of the style rules might make sense in the repository, to help new developers); if someone wants to use or contribute to django-daguerre, this slight deviation (which, out of the minority style rules in effect, is one of the most common) is easily handled, and for those for whom it is insurmountable, I don't think it's much of a loss for us to lose that contributor. Even if we switched, there is likely some small minority of developers that would be just as unwilling to contribute to a space-using or hard-wrapping project.

This is a case where the current state of things, however much you dislike it personally, takes precedence, and a substantive argument, where the benefits are clear to all sides and any penalty is similarly negligible, needs to be made such that the change becomes overwhelmingly preferred. I believe that since there's strife on this matter at all, the safest thing is to just leave it as is.

[melinath]

The whole point of making this project open source is to make it available and accessible to other developers. I'd be curious to know whether there's data on how many django-related projects use tabs; other than Oberlin's internal code, and Philo (which is essentially only used by Oberlin), I don't think I've ever seen one.

[melinath]

Also, just to add some perspective to this discussion:

django-daguerre$ find daguerre -iname "*.py" | xargs -L 1 git blame | wc -l
    2862
django-daguerre$ find daguerre -iname "*.py" | xargs -L 1 git blame | grep "Stephen" | wc -l
    2713
django-daguerre$ find daguerre -iname "*.py" | xargs -L 1 git blame | grep "Harris" | wc -l
     149
django-daguerre$ find daguerre -iname "*.py" | xargs -L 1 git blame | grep "Joseph" | wc -l
       0

I don't think we need to worry about git blame obscuring who authored a line. If it would make you feel better if I do the PEP 8-ifying, I'd be happy to do so.

[jspiros]

@melinath Regardless of who does it, I think it's a bad idea (and I've outlined some reasons why); so, no, it does not make me "feel better".

I'm not sure what perspective you're attempting to provide with your shell output. There is more than one author of the current master branch state, which would be obscured by the proposed change. Further, git blame is useful in two dimensions, and you're only even addressing one: it shows both the last author of a given line and the commit from which the line, as is, originates. Perhaps your argument is that those changes made by @harrislapiroff were or are insignificant, but even if I accepted that (which I do not), it's hardly insignificant to be able to tell when and in what context a given line originated.

You have yet to directly respond to any of the concerns that I have. I offered my concerns in response to a question, posed by you, as to whether this approach makes sense/is agreeable. ("Does that sound all right?" in the ticket description.) I don't think it does/is, and I've tried to make my concerns clear; if I've been unclear on any point, or if you have a counter-argument to anything, please let me know.

I'm a little unclear as to your motivation. I haven't seen a strong reasoned argument in favor of this change—not simply against git blame's significance—besides it simply being your personal preference (which itself seems to be based on an argumentum ad populum).

[jspiros]

Also, in the absence of any further constructive dialogue, if there is no end in sight, I defer to @harrislapiroff to simply make a decision one way or the other. This is a project under the aegis of the Oberlin College team, and I believe that makes Oberlin, or someone representing Oberlin's interests, the effective benevolent dictator in situations where one is required. As @harrislapiroff is the primary Oberlin contributor, I assume he effectively fills that role.

(And, indeed, my understanding of this being an Oberlin project, and my being involved now [and in the future] with the maintenance of Oberlin's web-related projects in general, is why I've been providing my perspective at all. I don't think that really matters, though, since I hope my arguments stand on their own, and my reason for providing them should have no basis on whether they represent truth. But, in case you were wondering...)

[melinath]

@jspiros: You raised two concerns: first, that it's a waste of time, and second, that git blame will be ruined. The second point is valid, in both dimensions, but we differ in how we weight that issue. My argument that spaces are more prevalent than tabs, particularly in the django ecosystem, is not an irrelevant one in this case, because the point of open sourcing this project was always to get other people to use the project and contribute. If we stick to conventions that people are used to, we're more likely to get contributions. In my opinion, that outweighs the temporary cost. In your opinion, it doesn't – which is why you call the proposal a waste of time. So then the question is, whose opinion do we want to weight more?

I have a couple of relevant points to make with that shell output.

First, I would point out that the original transfer was done by Harris. All previous history was lost. In other words, in the process of open sourcing the repository, git blame was utterly broken, and authorship of every line of code was ascribed to Harris. And perhaps it was a minor inconvenience. But since that time, not only have we somehow survived, but we've apparently rewritten at least every single line that's currently ascribed to me. So not only did we brazenly break git blame, it doesn't even matter any more because the entire codebase has been rewritten. And the entire codebase may be rewritten again. Open sourcing the project was the right thing to do to get the code out there, and I strongly believe that moving to a more strict PEP 8 is the right thing to do to help it spread. Yes, it may slightly inconvenience a few developers who are working specifically for Oberlin. Yes, it would probably spread even if we don't fix this, since the project is fucking awesome. But I think the project will be more easily accepted by more django developers if we stick to PEP 8, the same way I think it will be more easily accepted if we, say, use github rather than bitbucket.

My second point is that I am and always have been the primary contributor to this project by a long shot. The original internal Oberlin version was code I conceived and wrote; the idea to open source it came from me; and the vast majority of the work since then has been done by me (though I've definitely appreciated the help from Harris when he's had time to contribute.) Yes, this is technically under the aegis of Oberlin, but that's mostly just a matter of convenience, since the original code was split off of an internal Oberlin repository. In terms of conception and drive, this has always been primarily my project. I've done what I wanted when I wanted to do it. I've always asked for feedback from @harrislapiroff and taken that into consideration, because I have a lot of respect for him, but ultimately I have been the one making the design decisions for this project. If he wants to use the legal ownership of the project to take that away from me, I suppose that's his right.

As it stands, I remain unconvinced and would still like to see this project move to full PEP 8.

[jspiros]

Thanks for taking the time to reply so comprehensively.

Indeed, I still think you're overestimating the effect the current style would have on potential contributions. I don't think it matters one way or another, so I don't see the potential benefit you describe. And, I would prefer keeping things as they are unless it's absolutely necessary and beneficial to change. Also, if anything, on some level, I think that any community around a project that even brings this stuff up is somewhat unwelcoming or unstable, and would give me pause. But that could very well just be me.

It's unfortunate that Harris didn't transfer the repository better, I was unaware of that. That said, it doesn't really change the fact that it still adds to the problem, that fewer mass-changes by a single author/commit is to be preferred. "Two wrongs don't make a right", though, your argument seems to be something like "if a right is no longer possible, let's not worry about devaluing wrongs". Personally, I'm not compelled by that, since I otherwise don't think it matters one way or another and still don't see any benefit.

I think the salient point out of everything, though, is that you feel ownership of the project. I understand, then, why you think your personal preference should implicitly hold more weight. I don't agree (that that's the right way to handle feeling ownership, not that you feel it at all), necessarily, but I understand.

The problem with it being an Oberlin project, as I see it, is that it puts the burden of maintenance on Oberlin personnel; if it goes unmaintained, if bugs aren't fixed, it reflects poorly on Oberlin. Given that Oberlin has an implicit maintenance burden on all of its own projects, I do think it has authority in these cases. And, if it were up to me (and at one point it was, and at another point may yet again), all Oberlin projects would follow the same style guidelines, which would include tabs-instead-of-spaces and no-hard-line-wrapping.

But, I think the real question is, does it need to stay an Oberlin project? Why isn't the blessed repository on your account? Just because Oberlin owns the copyright on the code, since it's (apparently) MIT licensed, the blessed repository needn't be under Oberlin's account. You could fork it (conceptually), and then Oberlin could bless your repository as the official one by deleting its own and forking (GitHub-style) yours.

@harrislapiroff, thoughts?

[harrislapiroff]

Hoooooo boy, stuff got intense. @melinath and I have already discussed the possibility of moving Daguerre out of Oberlin's hands, though we haven't taken action on that yet. As far as I'm concerned, legal and GitHub ownership notwithstanding, Daguerre mostly (basically entirely) belongs to @melinath in terms of code and design, so as far as I'm concerned major architectural decisions belong to him as our sort of de facto BDFL. I don't think there's anyone else here at Oberlin to disagree. We'll make this arrangement formal in one way or another in the future.

I still disagree with @melinath about switching to spaces over tabs—I'm not persuaded by his arguments—but in light of the above, I will leave the decision to him and abide by it.

[jspiros]

As with @harrislapiroff, I'm still personally unpersuaded by @melinath's arguments. But, I think there's a point where the argument needs to stop and someone needs to make a decision, so someone needs to have the authority to do so. Given the situation, @melinath seems the natural choice, but it's confused by the fact that this is an @oberlin project.

So, I've been in touch with both @harrislapiroff and @melinath via e-mail to discuss and handle the potential formal transfer of this project away from the @oberlin team to @melinath. I've provided what advice I can on how to proceed, and it's my hope that the transfer will occur in time for the 1.0 release.

[melinath]

I do want this to land after #41... it needs to come in at a time when no other tickets are being worked on, or else it'll be a PITA.

[melinath]

@philipkimmey Now that #41 is merged, I'd like to get this done ASAP. What kind of timeline do you have for this?

[philipkimmey]

I've already started but I sidelined it during the discussion in case that ultimately wasn't the route desired.

I should be able to have a pull request good to go by the end of the week. On Mar 31, 2013 4:05 PM, "Stephen Burrows" notifications@github.com wrote:

@philipkimmey https://github.com/philipkimmey Now that #41https://github.com/oberlin/django-daguerre/issues/41is merged, I'd like to get this done ASAP. What kind of timeline do you have for this?

— Reply to this email directly or view it on GitHubhttps://github.com/oberlin/django-daguerre/issues/37#issuecomment-15699497 .

[melinath]

Sounds great.

[harrislapiroff]

@philipkimmey How's this coming along?

[philipkimmey]

Hey there - sorry I haven't made any progress. It's been a busy week.

I've slotted some time tomorrow & Friday - I think I should be able to knock it out then. On Apr 10, 2013 8:54 AM, "Harris L." notifications@github.com wrote:

@philipkimmey https://github.com/philipkimmey How's this coming along?

— Reply to this email directly or view it on GitHubhttps://github.com/oberlin/django-daguerre/issues/37#issuecomment-16183249 .

[harrislapiroff]

Sounds great. Glad to have you helping out!

On Apr 10, 2013, at 7:22 PM, Philip Kimmey notifications@github.com wrote:

Hey there - sorry I haven't made any progress. It's been a busy week.

I've slotted some time tomorrow & Friday - I think I should be able to knock it out then. On Apr 10, 2013 8:54 AM, "Harris L." notifications@github.com wrote:

@philipkimmey https://github.com/philipkimmey How's this coming along?

— Reply to this email directly or view it on GitHubhttps://github.com/oberlin/django-daguerre/issues/37#issuecomment-16183249 .

— Reply to this email directly or view it on GitHub.

[philipkimmey]

Wanted to update here - this has taken me a bit longer than I'd anticipated. This project is a bit bigger than I'd realized.

The first set of fixes is in 26d0b4ce6cc0d6d55ddc0a4f13d54e81b0da7ea7 .

Here's the current state of affairs:

Excluding migrations, docs and tests things look good:

lizard-king:django-daguerre philip$ find . -type f \( -name "*.py" ! -wholename "*migrations/*.py" ! -wholename "*docs/*" ! -wholename "*tests/*" \) -exec pep8 {} \; | wc -l
       0

I think you definitely don't want to both with PEP8 for migrations (since they're mostly a mish-mash of frozen models state), and I suspect the docs folder probably can be left alone since it's mostly autogenerated stuff (I think?). That leaves tests/*.

lizard-king:django-daguerre philip$ find . -type f \( -name "*.py" ! -wholename "*migrations/*.py" ! -wholename "*docs/*" \) -exec pep8 {} \; | wc -l
     659

I may be able to get to the remaining issues in the next few days, but I can't really promise it.

[melinath]

Agreed that tests need to be done, and that migrations could be excluded (though generated migrations are actually PEP8 compliant. If ours aren't, it's because they were intentionally changed.)

Docs can also be excluded for now, for sure.

[philipkimmey]

Generated migrations often exceed the 80 cols limit I believe. On Apr 13, 2013 10:00 PM, "Stephen Burrows" notifications@github.com wrote:

Agreed that tests need to be done, and that migrations can be excluded (though generated migrations are actually PEP8 compliant. If ours aren't, it's because they were intentionally changed.)

Docs can also be excluded for now, for sure.

— Reply to this email directly or view it on GitHubhttps://github.com/oberlin/django-daguerre/issues/37#issuecomment-16346062 .

[melinath]

Hey, @philipkimmey - just wanted to check in on this. I'd like to finalize the release this weekend; this ticket is the only one that's currently outstanding. Can you commit to getting this done by Sunday? If not, I can set aside the time to finish it up.

[melinath]

Resolved with a18b862105bfdea5288939c8c72b31a42c7a77a9.