HSEARCH-1292 Add rule to checkstyle for spaces at the end of lines #391

Closed
wants to merge 4 commits into
from

Projects

None yet

3 participants

@Sanne
Hibernate member

can you add another rule in checkstyle & jenkins please:

if a pull request touches more than 100 files, reject it right away
@Sanne
Hibernate member
@DavideD
Hibernate member

I cannot think of an alternative to make the rule less pedantic at the moment, so I guess you can close this pull request.

@hferentschik

I don't think we really need the license header on xml files. We confirmed that with Richard as well. IMO the ratio of useful lines to license size is just wrong! Just saying

@hferentschik
Hibernate member

if a pull request touches more than 100 files, reject it right away

Come on, either we want to get to a consistent style or not. If we do and whitespaces is one of the things we want to enforce, then a change of this sort is required. In case you are concerned about diffs, most diff tools can ignore the whitespace differences. For my part this change makes sense.

@hferentschik
Hibernate member

My 0.02$ at least

@Sanne
Hibernate member

sorry guys my idea "if a pull request touches more than 100 files, reject it right away" was not serious!
Just my first reaction open opening :D

I'm not strict on the license either: I would have preferred to apply this without changing the license since we clearly had whitespace in the template but if there is no easy way around I think we can consider this.

I'm wondering however if this whitespace rule should not take advantage of the parsing capabilities of checkstyle - which is contextual - rather than using a regex.

@Sanne
Hibernate member

generally speaking this definitely is the right moment to apply fixes, as we're about to start several code refactorings and AFAIK nobody started yet, with the exception of my Lucene4 stuff but I think that can be handled.

@DavideD if you want to go ahead and gradually enforce more rules this could be the best week to do so

@DavideD
Hibernate member

I don't think we really need the license header on xml files. We confirmed that with Richard as well. IMO the ratio of useful lines to license size is just wrong! Just saying

I agree but It seems that most of the xml files have it. I wanted to be consistent. Anyway, I can remove it from this pull request.

I'm wondering however if this whitespace rule should not take advantage of the parsing capabilities of checkstyle - which is contextual - rather than using a regex.

I haven't found any existing module to add to checkstyle for this particular case but if someone has alternatives we can test them.

generally speaking this definitely is the right moment to apply fixes, as we're about to start several code refactorings and AFAIK nobody started yet, with the exception of my Lucene4 stuff but I think that can be handled.

Maybe I can change the jira connected to this issue to be more generic and apply all the new rules under the same issue. I think all the other rules will have less impact on the code base.

@Sanne
Hibernate member

Maybe I can change the jira connected to this issue to be more generic and apply all the new rules under the same issue. I think all the other rules will have less impact on the code base.

As you prefer. Feel free to create additional JIRAs and proceed in smaller steps if that works bettter. Commit-wise I always prefer smaller patches: makes conflict resolution easier.

@hferentschik
Hibernate member

Maybe I can change the jira connected to this issue to be more generic and apply all the new rules under the same issue. I think all the other rules will have less impact on the code base.

+1

@hferentschik
Hibernate member

I don't think we really need the license header on xml files. We confirmed that with Richard as well. IMO the ratio of useful lines to license size is just wrong! Just saying

I agree but It seems that most of the xml files have it. I wanted to be consistent. Anyway, I can remove it from this pull request.

True. I guess there is the consistency argument. Just saying it is not really needed and maybe this new "type" of xml file is a good point to break w/ convention. But if you want to keep it, keep it.

@DavideD
Hibernate member

We are not integreting this pull request for nw
@Sanne is adding some more checkstyle rules and I will change the jira to be less specific so that we don't need to create a new JIRA for every new check

@DavideD DavideD closed this Apr 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment