Skip to content
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

Clean up all whitespace throughout project #5578

Merged
merged 1 commit into from Nov 9, 2017
Merged

Clean up all whitespace throughout project #5578

merged 1 commit into from Nov 9, 2017

Conversation

jdufresne
Copy link
Contributor

@jdufresne jdufresne commented Nov 9, 2017

  • 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.

Copy link
Collaborator

@carltongibson carltongibson left a comment

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?

@jdufresne
Copy link
Contributor Author

jdufresne commented Nov 9, 2017

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.

@carltongibson
Copy link
Collaborator

carltongibson commented Nov 9, 2017

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!

@@ -1,7 +1,7 @@
# SOME DESCRIPTIVE TITLE.
# Copyright (C) YEAR THE PACKAGE'S COPYRIGHT HOLDER
# This file is distributed under the same license as the PACKAGE package.
#
#
Copy link
Member

@rpkilby rpkilby Nov 9, 2017

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.

Copy link
Collaborator

@carltongibson carltongibson Nov 9, 2017

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.
@jdufresne
Copy link
Contributor Author

jdufresne commented Nov 9, 2017

Updated to remove changes to *.po files.

rpkilby
rpkilby approved these changes Nov 9, 2017
@carltongibson carltongibson added this to the v3.7.4 milestone Nov 9, 2017
@carltongibson carltongibson merged commit f9c67f0 into encode:master Nov 9, 2017
1 check passed
@jdufresne jdufresne deleted the whitespace branch Nov 10, 2017
@anx-ckreuzberger
Copy link
Contributor

anx-ckreuzberger commented Nov 14, 2017

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.

@rpkilby
Copy link
Member

rpkilby commented Nov 14, 2017

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

@anx-ckreuzberger
Copy link
Contributor

anx-ckreuzberger commented Nov 14, 2017

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants