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

Remove all trailing whitespace #1010

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

stweil
Copy link
Contributor

@stweil stweil commented Mar 15, 2023

Use git log --check to see the commits which introduced trailing whitespace.

@kba
Copy link
Member

kba commented Mar 16, 2023

Not a big fan to be honest, because it updates

  • some files that haven't been updated in a long while, making it harder to see what has actually changed recently
  • changes generated files and XML schemas we import from elsewhere
  • requires changes to the tests because of changed checksums

Besides the aesthetics, is there a functional reason for this?

@bertsky
Copy link
Collaborator

bertsky commented Mar 16, 2023

I concur with @kba. Adding:

  • conflicts with outstanding PRs, making it harder to finalize and catch up

IMO we don't need these cosmetics, least now.

@stweil
Copy link
Contributor Author

stweil commented Mar 16, 2023

These objections are not new, and some of them are easy to solve (for example by ignoring whitespace changes). But there is a common consensus among most open source projects that these kinds of whitespace should not be part of the code base. That's the reason why Git marks commits with such whitespace issues and why git log --check reports them.

Ideally the code base should be cleaned, also in upstream projects. That makes it easier to maintain the code because editors then can avoid further whitespace issues without changing existing lines with such issues. It also reduces the size of an installation by a few bytes.

A minimal requirement should be a check for new contributions which marks those with issues as invalid. The Linux kernel provides a very powerful script which does that, but might not be usable because of the license conflict.

@stweil
Copy link
Contributor Author

stweil commented Jun 26, 2023

I rebased and updated this pull request. The bad news is that now 29 files show whitespace issues. Some months ago 26 files were affected.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Stefan Weil <sw@weilnetz.de>
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.

3 participants