New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Policy change: Forbid white-space changes and reformatting #2727

Open
htgoebel opened this Issue Aug 7, 2017 · 4 comments

Comments

Projects
None yet
1 participant
@htgoebel
Member

htgoebel commented Aug 7, 2017

I propose to forbid any white-space changes and reformatting in existing code - with the sole exception of "severe" cases. Even if a line is changed, the code should not be reformatted beyond what is required to keep the code in good style.

Implementation

  • The development-guideline will get a statement like this:

    Commits including white-space changes or reformatting will not be accepted. This is also true for commits consisting of only white-space changes or reformatting. The only exceptions are "severe" formatting issues or issues in strings. For rational please refer to issue #…(this one)

  • We try to enforce this rule.
  • We try to enforce some white-space rules (e.g. newline at end of file, no while-space at end of line).
  • We verify code-quality of any change. Only a few rules should be enforced here, since code-formatting often depends on

All of the aforementioned checks should be done as early as possible (using a hook, some online service or whatever).

Rational

Currently we accept white-space changes and reformatting, as long as they are kept in separate commits – as written in the development guideline. Some reasons for changing this rule:

  1. Time-consuming: It's time-consuming to review these commit and verify they are non-functional changes only.

  2. Makes it hard to merge or rebase: Every unrelated change makes it harder to merge the pull-request or to rebase the commits. Smaller commits contain less conflicts and conflicts are easier to solve in smaller commits.

  3. Bad in the long-run: Experience shows, that in the long-run both of these change make it hard to follow changes:

    • In git blame the "last change" for the lines effected will be a non-functional change. If one wants to know "Why has this line been changed?", he/she would need step through reformatting and white-space changes. This is simply one thing: disgusting. This can be eased a bit by using gut gui blame or the github weg-ui, but it still adds additional steps.
    • The same is true when using git log -p ot git log --grep, since these will yield additional entries which do not help to understand the code.
  4. There is no need for re-formatting again: PyInstaller's code quality and formatting are quite acceptable already. Larger re-formatting has been done in the past, so there is no urgent need for another re-formatting. The only thing I'm aware are some long lines, but all I'm aware of are still within an acceptable range.

  5. PEP-8 is a recommendation: PEP-8 is an important document, but it's only a recommendation, and often it recommends several alternatives. We already follow PEP-8, but obeying PEP-8 only for PEP-8's sake is idiocy. This would upraise PEP-8 to some kind of religious rule.

    Experience shows that often the code is easier to understand when not strictly following PEP-8.

  6. If we'd want to enforce a concrete formatting style, we'd need to provide a .pylint file (or something like this) defining our rules. Otherwise each developer would have his/her own notion of the "correct" formatting style. But again: This can only be recommendations.

  7. Why should the code be reformatted at all?

  8. Other project have this policy, too. I contributed reformatting changes to some other projects and they have been rejected. (Sorry, I can't remember which ones this have been.)

FAQ

  • Why not simply accept white-space changes and reformatting?
    See above.
  • Why not run a reformatting all over the code once (and forever)?
    We already did this several times, see git log --grep PEP8 -- PyInstaller.

@htgoebel htgoebel self-assigned this Aug 7, 2017

@htgoebel htgoebel changed the title from Policy change: Forbit white-space changes and reformatting to Policy change: Forbid white-space changes and reformatting Aug 7, 2017

@ghost

This comment has been minimized.

ghost commented Aug 7, 2017

How about adding pytest-flake8? That will resolve these problems once and for all.

[Edit by htgoebel: xoviat tested this in #2729 and was not satisfied with the result.]

@htgoebel

This comment has been minimized.

Member

htgoebel commented Aug 10, 2017

Added new rationals: "Makes it hard to merge or rebase" and "Other project have this policy, too".

@ghost

This comment has been minimized.

ghost commented Aug 29, 2017

So #2778 should resolve the formatting concerns once and for all.

@htgoebel

This comment has been minimized.

Member

htgoebel commented Sep 25, 2017

#2778 implements linting using flak8-diff (flake8 running on the diff only), which is a good thing.

A solution for checking for useless reformatting is still pending. Any ideas?

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