Proposal to end the war with flake8 warnings

[Tim Graham]

I'd like to propose cleaning up Django's codebase so that we can run flake8 (with some rules ignored) as a presubmit check (for example, hooked into pull request submissions).

Our docs currently state: "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 somewhat disagree, I think it's better to make cleanups in separate commits so that when looking at a commit, you don't need to figure out what changes are stylistic and what changes are needed for the fix.

The benefit of doing the cleanup now is that we could automate these style checks afterward which will make code review more efficient. As Alex wrote in his blog post on code review: "Don't use humans to check for things a machine can. This means that code review isn't a process of running your tests, or looking for style guide violations."

A drawback is that it will introduce some noise in the commit history in the short term and make git blame less efficient. I believe the long term benefit of not being at war with these issues is worth this trade-off.

If accepted, I'll put together a more concrete proposal that includes which errors we'll ignore, etc.

[Juan Pablo Martínez]

+1 hip hip cleanup!

On Wed, Oct 9, 2013 at 4:38 PM, Tim Graham <timog...@gmail.com> wrote:

things a

--
juanpex

[Anssi Kääriäinen]

+1

 - Anssi

[Daniele Procida]

On Wed, Oct 9, 2013, Tim Graham <timog...@gmail.com> wrote:

>Our docs currently state: "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
>somewhat disagree, I think it's better to make cleanups in separate commits
>so that when looking at a commit, you don't need to figure out what changes
>are stylistic and what changes are needed for the fix.

I certainly agree with this.

Daniele

[Aymeric Augustin]

On 9 oct. 2013, at 20:38, Tim Graham <timog...@gmail.com> wrote:

If accepted, I'll put together a more concrete proposal that includes which errors we'll ignore, etc.

Sure, go ahead!

I'm always embarrassed when contributors work on large cleanup patches, because they're especially tedious to review and don't get committed. However I support committers pushing such changes.

-- 

Aymeric.

[Carl Meyer]

On 10/09/2013 12:38 PM, Tim Graham wrote:
> I'd like to propose cleaning up Django's codebase so that we can run
> flake8 (with some rules ignored) as a presubmit check (for example,
> hooked into pull request submissions).
>
> Our docs currently state: "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

> somewhat disagree, I think it's better to make cleanups in separate
> commits so that when looking at a commit, you don't need to figure out
> what changes are stylistic and what changes are needed for the fix.
>
> The benefit of doing the cleanup now is that we could automate these
> style checks afterward which will make code review more efficient. As
> Alex wrote in his blog post on code review

> <http://alexgaynor.net/2013/sep/26/effective-code-review/>: "Don't use

> humans to check for things a machine can. This means that code review
> isn't a process of running your tests, or looking for style guide
> violations."
>
> A drawback is that it will introduce some noise in the commit history in
> the short term and make git blame less efficient. I believe the long
> term benefit of not being at war with these issues is worth this trade-off.
>
> If accepted, I'll put together a more concrete proposal that includes
> which errors we'll ignore, etc.

+1 from me.

And I agree that dedicated comprehensive style cleanup commits are much
better than mixing it in with functional commits "when you're in the
area"; I'd support changing that paragraph in our docs (though it
shouldn't be relevant anymore once you've done one comprehensive pass
and integrated an automated flake8 in the PR process).

Carl

[Krzysztof Jurewicz]

On 10/09/2013 08:38 PM, Tim Graham wrote:
>
> A drawback is that it will introduce some noise in the commit history in
> the short term and make git blame less efficient.

Note however that there is �-w� option for git blame, which does the
following: �Ignore whitespace when comparing the parent�s version and
the child�s to find where the lines came from.�

So that noise can be somewhat reduced, at least when using the command line.

[Christopher Medrela]

I've used autopep8 to make some additional cleanup -- see this pull request: https://github.com/django/django/pull/1875