search

editorconfig / editorconfig.github.com

Configuration file format for defining coding styles in shared projects
http://editorconfig.org
Other
265 stars 42 forks source link

Whitespace improvements #143

Closed kmk3 closed 3 years ago

kmk3 commented 3 years ago 
$ git log --reverse --pretty='* %s' -s master..HEAD
* Add .gitattributes
* Fix whitespace with git stripspace
* .editorconfig: enable trim trailing ws / insert final newline
* .editorconfig: define consistent charset and eol
kmk3 commented 3 years ago

@xuhdev Hello, would you mind reviewing this?

If something does not seem right/seems unclear feel free to say so.

If the changes are too many I can split it.

xuhdev commented 3 years ago

Thanks again for your contribution!

kmk3 commented 3 years ago

Sorry I missed this one earlier. Yes, I think it's good improvement.

No problem; thanks.

Thanks again for your contribution!

Thanks for the review, but please don't use GitHub's "squash and merge" option in general when there is more than one commit involved. I usually try to make one commit for each logical change (as per [1]) so that it's easy to understand and blame/bisect (and replicate if the changes are made by a script) and squashing it all takes away these benefits and the work put into rebasing/organizing the commits before opening a PR...

As for the alternatives, using any of the other options (i.e.: a normal merge or "rebase and merge") works fine. Unless a branch consists mostly of low quality/amend commits (which likely should have been squashed beforehand anyway, etc), please ask before squashing.

[1] https://chris.beams.io/posts/git-commit/

xuhdev commented 3 years ago

Got it, I didn't notice you have organized the commits. Will do rebase for your PRs in the future.

kmk3 commented 3 years ago

Got it, I didn't notice you have organized the commits. Will do rebase for your PRs in the future.

Thanks! If it's not too much to ask, would you mind undoing the merge and redoing this in a new PR? I think the following commands would be enough:

git checkout master
git stash # just in case
git pull
git reset --hard fca165c
git push --force-with-lease origin master
xuhdev commented 3 years ago

@kmk3 We don't do force pushing (we don't know who else is relying on the git history). However, I can push a reversion of this commit. Would that be OK for you?

kmk3 commented 3 years ago

@kmk3 We don't do force pushing (we don't know who else is relying on the git history). However, I can push a reversion of this commit. Would that be OK for you?

I see; that works too. Mind if the revert is done through #149?

(I like to make sure that git repos are self-sufficient, so I put the rationale on the revert commit itself)

xuhdev commented 3 years ago

I merged #149. Feel free to open another PR to redo this contribution :)