Merge policy for cleanup commits

[Danilo Bargen]

Hi there

(I tried to find previous threads concerning the same topic, but could not find any. If this is already the n-th discussion about the topic, sorry about it.)

I don't quite agree with the cleanup commit policy stated in the docs:

Systematically remove all trailing whitespaces from your code as those add unnecessary bytes, add visual clutter to the patches and can also occasionally cause unnecessary merge conflicts. Some IDE’s can be configured to automatically remove them and most VCS tools can be set to highlight them in diff outputs. Note, however, that patches which only remove whitespace (or only make changes for nominal PEP 8 conformance) are likely to be rejected, since they only introduce noise rather than code improvement. Tidy up when you’re next changing code in the area.

I did such a PR today (without knowing that policy): https://github.com/django/django/pull/1345

While I agree that small PRs which fix issues like whitespace should not necessarily clutter up the commit history, I disagree for larger cleanup commits. In some places the code has serious formatting issues (e.g. lines that are indented 3 instead of 4 characters and that only work thanks to the lax Python indentation parsing). Also, I disagree that 1 commit which cleans up a file would "clutter" its commit history.

While I support fixing coding standard issues on-the-go, I think that it clutters up the commit history in a worse way than a cleanup commit would, because the commits are not strictly focused on the feature or bug they concern.

In addition to the PEP8 changes there were also a few pyflakes changes that go beyond simple whitespace formatting. For example in the core module there were a few places where "variable == None" was used, even though "variable is none" should be used preferredly. Another issue would be "type(c) == Typeclass" instead of "isinstance(c, Typeclass)".

Anyways, if you don't want to accept such commits that's OK, but I think adherence to coding standards is pretty bad in many Django modules and it should be fixed. And for sure I won't be the last person to send you such a pull request.

Danilo

[Aymeric Augustin]

On 8 juil. 2013, at 18:59, Danilo Bargen <gez...@gmail.com> wrote:

> While I agree that small PRs which fix issues like whitespace should not necessarily clutter up the commit history, I disagree for larger cleanup commits. In some places the code has serious formatting issues (e.g. lines that are indented 3 instead of 4 characters and that only work thanks to the lax Python indentation parsing).

I can't speak for other core devs, but I won't merge such PRs for a very simple reason: it's more tedious and time-consuming to review them than to redo them by myself.

If someone took advantage of a huge "style cleanup" diff to slip in a security vulnerability — and trust me, it doesn't take much code — I wouldn't want to have my name on the commit.

> Anyways, if you don't want to accept such commits that's OK, but I think adherence to coding standards is pretty bad in many Django modules and it should be fixed.

Like the 1400 or so tickets currently open in Trac :)

> And for sure I won't be the last person to send you such a pull request.

You aren't the first one either. For some reasons I don't quite understand, "hey, your coding standards suck, mine are better" is a common first-contact technique :)

--
Aymeric.

[Marc Tamlyn]

I have to agree with Aymeric, despite being one of those people who wanted to fix this at times in the past.

It is worth remembering that some areas of the Django code base are very old, and python coding standards in the community were not as strong when the code was written. Believe it or not, the situation is slowly improving over time and the amount of ugly looking code is decreasing. Over time, it will go away and new contributions are encouraged to match up to the standards now.

The most important thing to remember though is that this is code which works, which is fundamentally the most important thing.

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

[Danilo Bargen]

> I can't speak for other core devs, but I won't merge such PRs for a very
> simple reason: it's more tedious and time-consuming to review them than to
> redo them by myself.

Even though there are many changes, the changes are very obvious and
simple and quick to review.

> If someone took advantage of a huge "style cleanup" diff to slip in a
> security vulnerability — and trust me, it doesn't take much code — I
> wouldn't want to have my name on the commit.

That's right. But if you scroll through the diff on Github it's very
easy to see that there are no "deep" changes... Probably 95% of the
changes are whitespace anyways.

> Like the 1400 or so tickets currently open in Trac :)

A cleanup doesn't mean that other tickets should not be fixed :)

> You aren't the first one either. For some reasons I don't quite
> understand, "hey, your coding standards suck, mine are better" is a common
> first-contact technique :)

Well, it's actually your coding standard:
https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/
And it's not my first contribution to Django either. It's just
something that apparently bugs many people and that would be easy to
fix.

> The most important thing to remember though is that this is code
> which works, which is fundamentally the most important thing.

Yes, practicality beats purity, but at the same time beautiful is
better than ugly, sparse is better than dense and readability counts
:)

Cheers
Danilo

[Tomas Ehrlich]

Relevant discussion
https://groups.google.com/forum/#!msg/django-developers/tcNVvbAv4-M/bs0zPNLqv48J


Cheers,
Tom

Dne Mon, 8 Jul 2013 09:59:49 -0700 (PDT)
Danilo Bargen <gez...@gmail.com> napsal(a):

> Hi there
>
> (I tried to find previous threads concerning the same topic, but could not
> find any. If this is already the n-th discussion about the topic, sorry
> about it.)
>
> I don't quite agree with the cleanup commit policy stated in the docs:
>
> Systematically remove all trailing whitespaces from your code as those add
> > unnecessary bytes, add visual clutter to the patches and can also
> > occasionally cause unnecessary merge conflicts. Some IDE’s can be
> > configured to automatically remove them and most VCS tools can be set to
> > highlight them in diff outputs. Note, however, that patches which only

> > remove whitespace (or only make changes for nominal PEP 8<http://www.python.org/dev/peps/pep-0008>conformance) are likely to be rejected, since they only introduce noise

[Danilo Bargen]

Hm, I'm not sure I understand your e-mail. The link you just posted points to exactly the topic we're discussing right now. Maybe you meant to post another URL?

Cheers,

Danilo