Clean up all whitespace throughout project by jdufresne · Pull Request #5578 · encode/django-rest-framework (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation9 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

• Remove trailing whitespace from lines
• Remove trailing nad leading whitespace from files

Allows for cleaner diffs in future changes. For editors that automatically clean up whitespace on save, will avoid unrelated line changes in diffs.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. This looks good. The sort of thing I'm all for really...

1. Could we add an editor config file to help force editors to behave nicely. (1a: Is the support for that widespread enough to make it worth it?)
2. Do we want to enforce this at the lint step?

Could we add an editor config file to help force editors to behave nicely.

Added an .editorconfig file with the following configuration:

root = true

[*] charset = utf-8 end_of_line = lf insert_final_newline = true trim_trailing_whitespace = true

Do we want to enforce this at the lint step?

I'm open to this idea. Did you have a particular tool in mind? I'm not aware of one that checks across all these different file types for whitespace issues. So this has not yet been added.

Did you have a particular tool in mind?

No. 😃

I'm in two minds about it. I suspect we'd end up with a lot of failures — we get plenty with flake8. That may not be a bad thing, but we could probably live without enforcing it, since every second person would be setting the right whitespace on save. (As I say, two minds.)

I'll leave this open for a bit, to see if there are any comments, otherwise we'll have this I think. Thanks for the input!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with transifex so I might be wrong here, but aren't the .po files regenerated by transifex for every release? It seems like the whitespace changes there would be superfluous.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpkilby Yes. That's we all the po files and the compilemessages from those. Good catch.

They won't ever be hand edited I'd guess.

• Remove trailing whitespace from lines
• Remove trailing nad leading whitespace from files

Allows for cleaner diffs in future changes. For editors that automatically clean up whitespace on save, will avoid unrelated line changes in diffs.

Updated to remove changes to *.po files.

I have enforced this with flake/pep on a project at work, and I've had almost no issues with this.
I think you can do an automated independent code quality test for PR on github.

Hi @anx-ckreuzberger. Python files are already covered by flake8. Does your config include markdown or other non-python files?

Ah, I guess I misunderstood the comment from above.

I actually do not lint other non-python files, though I do use a little "bigger" editor config:

root = True

# Unix-style newlines with a newline ending every file
[*]
end_of_line = lf
insert_final_newline = true
indent_style = space
indent_size = 4

# Matches multiple files with brace expansion notation
# Set default charset
[*.{js,html}]
charset = utf-8

# Matches the exact files either package.json or .travis.yml
[{package.json,.travis.yml}]
indent_style = space
indent_size = 2

[*.less]
indent_size=2

# Ignore everything for all stuff under node_modules, docker and public
[{node_modules/**,docker/**,public/**}]
charset = none
end_of_line = none
insert_final_newline = none
trim_trailing_whitespace = none
indent_style = none
indent_size = none

I also use eslint for linting JavaScript, though I guess that's a little too much for DRF.

pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request

Nov 17, 2020

• Remove trailing whitespace from lines
• Remove trailing nad leading whitespace from files

Allows for cleaner diffs in future changes. For editors that automatically clean up whitespace on save, will avoid unrelated line changes in diffs.