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

ISPN-5683 Removed whitespaces at the end of the lines #4513

Merged

Conversation

slaskawi
Copy link
Contributor

https://issues.jboss.org/browse/ISPN-5683

Please do not close the issue once this PR gets in. I'm turning on the rules gradually.

Fixed by a simple command:

find . -name "*.java" -exec sed -i 's/\s*$//' {} \;

@rvansa @anistor @galderz @Sanne Could you please review it?

@rvansa
Copy link
Member

rvansa commented Aug 17, 2016

-1 I use git annotate (blame) to crawl through history quite often, and this introduced unwanted churr.

@slaskawi
Copy link
Contributor Author

@rvansa Hey Radim! Just to be crystal clear - the alternative is to skip this rule.

I assume you are -1 for submitting this change and +1 for removing the rule for checking empty char at the end of the line?

@anistor
Copy link
Member

anistor commented Aug 17, 2016

http://bit.ly/29oQu40 💔 😞

@rvansa
Copy link
Member

rvansa commented Aug 17, 2016

@slaskawi Yep. My priority list is : nice history > no invisible chars on the end of lines > invisible chars on the end of lines

Is it possible that such post-processing would be rather a part of a commit hook, affecting only diffs?

@slaskawi
Copy link
Contributor Author

I have an idea - would you prefer to have this one or #4515 ? Let the vote begin!

@slaskawi
Copy link
Contributor Author

👎 from me

@anistor
Copy link
Member

anistor commented Aug 17, 2016

If we give up on this then we might as well give up on the remaining ~35 rules. -1 for giving up.

@anistor
Copy link
Member

anistor commented Aug 17, 2016

By looking at the changes I see that the majority are on empty lines or javadocs, and few on actual statements. There will be some noise in history indeed, but not where it hurts the most. I'd allow this. And would even go as far as activate the manadatory new-line at EOF rule too so we are spared from seeing these: https://github.com/infinispan/infinispan/pull/4513/files#diff-e2e6f183514f1de4a3e41498851f8a46L6

@slaskawi
Copy link
Contributor Author

@anistor has a point here. If we give up one simple rule like this one, we might give up all of them. Also if you use formatter, you should never see this violation in practice. So changing my mind, 👍 from me.

@tristantarrant
Copy link
Member

My vote is with Adrian on this one

On 18 Aug 2016 07:56, "Sebastian Łaskawiec" notifications@github.com
wrote:

@anistor https://github.com/anistor has a point here. If we give up one
simple rule like this one, we might give up all of them. Also if you use
formatter, you should never see this violation in practice. So changing my
mind, 👍 from me.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#4513 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAX9Hmq1MnlBNDGWIDiXp-_lXqTSWV_nks5qg_P8gaJpZM4JmZ-O
.

@rvansa
Copy link
Member

rvansa commented Aug 18, 2016

Okay, I admit I haven't checked all the files - if it's mostly javadoc, it probably won't hurt.

@ryanemerson
Copy link
Contributor

+1 It will be worth the initial pain in the long run.

@slaskawi slaskawi force-pushed the ISPN-5683/Enable_additional_rules branch from 212f75f to 3942df3 Compare August 22, 2016 08:19
@slaskawi
Copy link
Contributor Author

Rebased

@anistor
Copy link
Member

anistor commented Aug 22, 2016

Looking again and pulling...

@anistor anistor merged commit 3942df3 into infinispan:master Aug 22, 2016
@anistor
Copy link
Member

anistor commented Aug 22, 2016

Integrated. Thanks @slaskawi !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants