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

WIP: removal of trailing whitespace #257

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

Conversation

a1346054
Copy link
Contributor

No description provided.

@Nikratio
Copy link
Contributor

Thank you! The problem with white-space fixes is that they make git blame much harder to use and there is nothing that prevents trailing whitespace from being introduced again in the future. So I would prefer if you could revert those.

@ccope
Copy link
Contributor

ccope commented Aug 23, 2021

Would you take the whitespace fixes if we added a CI step to check for them? I've been hacking on an unrelated feature myself and my editor keeps complaining about them.

@Nikratio
Copy link
Contributor

Yeah, I guess so. But please still put them in a separate commit.

@a1346054
Copy link
Contributor Author

I separated the pull request into individual commits, and additionally added an .editorconfig file so that trailing whitespace doesn't happen in the future.

@ccope
Copy link
Contributor

ccope commented Aug 24, 2021

I'll look into setting up a CI step today to enforce code style.

@ccope
Copy link
Contributor

ccope commented Aug 24, 2021

I'd also hold off on merging this for the moment, we can bundle any/all formatting fixes together and then put the commit hashes in a .git-blame-ignore-revs file

Actually never mind, we can add the formatting commits to the ignore file whenever

@Nikratio
Copy link
Contributor

I don't think the .editorconfig file will be sufficient to ensure that this does not recur. There's too many editors that are not using it (including Emacs).

Can we split this into one pull request for whitespace + ignore file + CI changes, and a second one for the other changes?

@a1346054
Copy link
Contributor Author

Split into #258

@a1346054 a1346054 changed the title fix shellcheck identified issues, spelling and whitespace WIP: removal of trailing whitespace Aug 25, 2021
@ccope ccope mentioned this pull request Aug 28, 2021
3 tasks
@a1346054
Copy link
Contributor Author

Rebased on latest master.

Leaving this PR open in case the .editorconfig file is considered useful.

Also still looking into how to add these particular fixes in an automated way.

Copy link
Contributor

@kalvdans kalvdans left a comment

Choose a reason for hiding this comment

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

I think this can be merged independently of a repo-wide reformatting and CI check.

Removed lines won't show up in git blame :)

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.

4 participants