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

Trim trailing whitespace #832

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

a1346054
Copy link
Contributor

No description provided.

danielshahaf pushed a commit that referenced this pull request Aug 20, 2021
@danielshahaf
Copy link
Member

@a1346054 Thanks. I've cherry-picked the spelling fixes with a tweaked log message. I don't particularly mind trailing whitespace, but leaving this open to give other devs a chance to weigh in. Thanks!

@a1346054
Copy link
Contributor Author

since an .editorconfig file is present, it would solve all future trailing whitespace issues if

trim_trailing_whitespace = true

were added to it.

Trailing whitespace makes my editor shout at me :)

@a1346054 a1346054 changed the title Minor cleanup Trim trailing whitespace Aug 31, 2021
@phy1729
Copy link
Member

phy1729 commented Sep 18, 2021

Not all editors honor .editorconfig though I have no objection adding that line. I'm ok with killing all the trailing whitespace as long as it's in one commit (which it is). Would be nice to add something like git grep -I ' $' as a CI test to ensure no trailing whitespace sneaks in in the future.

@danielshahaf
Copy link
Member

Users of editorconfig-less editors might be able to use a gitattributes(5) clean filter instead?

I don't see why we do this at all, as opposed to calling this a bug in the OP's editor or its configuration. If editorconfig support were ubiquitous, that'd be one thing, but since it isn't—well, throwing CI failures at people because their $EDITOR doesn't have editorconfig support (installed, or at all) doesn't sound to me like a good use of volunteer time :(

@phy1729
Copy link
Member

phy1729 commented Sep 19, 2021

I'm not strongly attached to the idea, but in most cases I think it's better to have a linter/CI system point out issues rather than a human reviewer. It lessens the cycle time to feedback. +1 to the PR, but also wouldn't object to a NACK.

@danielshahaf
Copy link
Member

Agreed that whenever there's an issue to be pointed out, lessening the time to feedback is desirable. (Which, incidentally, is an argument for pushing the validation into make check.) My argument is that 'Your $EDITOR doesn't honour the editorconfig trim_trailing_whitespace setting' isn't an issue that should be pointed out at all. Insisting on it would add a barrier to entry for some contributions; I think this cost would outweigh the gains. 

@phy1729
Copy link
Member

phy1729 commented Sep 20, 2021

Do you suggest the policy be accept a PR with trailing whitespace as is or fixup the PR when merging without troubling the submitter about it?

@danielshahaf
Copy link
Member

Yes, preferably the former. WDYT?

@phy1729
Copy link
Member

phy1729 commented Sep 23, 2021

I prefer the latter as I dislike trailing whitespace, but can accept doing the former as well.

@danielshahaf
Copy link
Member

So we have one +1 and one +0 for each of two mutually-exclusive options.

Do we make decisions by consensus or by majority? If we make decisions by consensus, the short answer is that we don't have consensus. If we make decisions by majority, we can ask Julien to cast a deciding vote.

I'd rather we were a consensus project. Quoting producingoss:

In general, taking a vote should be very rare — a last resort for when all other options have failed.

@a1346054
Copy link
Contributor Author

a1346054 commented Sep 3, 2022

Rebased to fix merge conflict.

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