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

Add checkstyle [SPR-16968] #21506

Closed
spring-projects-issues opened this issue Jun 21, 2018 · 10 comments
Closed

Add checkstyle [SPR-16968] #21506

spring-projects-issues opened this issue Jun 21, 2018 · 10 comments
Assignees
Labels
type: task
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Jun 21, 2018

Phil Webb opened SPR-16968 and commented

Adding checkstyle to the Spring Framework build would help catch common coding issues and aid contributors.


Referenced from: pull request #1865

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 21, 2018

Phil Webb commented

I have a PR in progress

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 21, 2018

Phil Webb commented

#1865

First off, a massive apology for the size of the PR. It turned out to be a lot more involved that I first thought. Other than the number of files touched, I don't think there's too many controversial parts to the PR. Perhaps the one that might cause the the biggest concern is the change in imports. I've added rules to enforce the import order based on the rules in the WIKI but I've also enforced that star static imports shouldn't be allowed. This was mainly so I could use IDE tooling, but I also think it makes sense as it's very easy to mess up if the same method name is declared in more than one star import. I've had this happen quite often with Mockito and Hamcrest. I also think it's easier to read the source offline when when exact import statements are declared.

The other one that might be a bit controversial is single argument lambdas. In Spring Boot we opted to always use the parentheses around the argument so that single argument and multi-argument lambdas look the same.

Let me know if you want me to refine any of the rules or rework the commits in any way.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 22, 2018

Juergen Hoeller commented

Phil, there is plenty of good polish in that PR but it also adapts a few widespread things that we intentionally did differently: e.g. no copyright headers on package-info, no "this." prefix on logger access, no type+"this." prefix in inner classes for access to outer state... and allowing star imports in test classes.

Generally speaking, I'm not sure whether enforcing production checkstyle guidelines on test sources is necessary or even desirable. At the very least, we could have more relaxed guidelines for test sources. For a start, enumerating the assertion method imports for every single test class feels odd to me... but I'm also generally much more relaxed about the code style used for test methods or inner classes used by them, with many production rules not needing to apply there.

As for single-argument lambdas, I visually object to the enforced parentheses there. Frankly, I'd rather wrap the expression itself in additional parentheses than such a plain parameter name part. We knew you chose differently in Boot but I'm afraid I'm not up for changing our style here in this respect.

The equals/hashCode stuff is unfortunately a common false signal by style-checking tools. All of those classes were valid since the superclass hashCode() implementation has a base hash that the subclasses choose to keep in that coarse-grained fashion. Enforcing hashCode() in those subclasses seems rather pointless, in particular when the overridden methods literally just call super and are effectively just there to keep checkstyle happy.

With respect to consistent code blocks and consistent ternary expression style, there were a few glitches... but also a few cases where we intentionally used one-line if variants, e.g. in AbstractBeanDefinition for better readability of that long if cascade. I'm also not opposed to shortened blocks in test classes, and I don't see all those inverted ternary expressions as really worthwhile... when local readability doesn't really improve through that measure.

All in all, from where I stand at the moment, the PR itself is way too much to merge. Let me rather go through the commits individually and hand-pick key changes while ignoring a whole range of others... and see where we end up with. In particular, I'm going to focus on production source fine-tuning, not touching test sources unless there is something totally bogus there. Once I've done an initial pass through this, let's see which checkstyle rules we'll eventually set up.

Last but not least, thanks for your efforts there! This is pointing out a lot of stuff worth reviewing, even in areas where we might not enforce hard rules eventually.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 22, 2018

Phil Webb commented

Thanks for the fast review, I know there's a way too much in one PR so I was expecting to need a few rounds. I might be able to do something with the commits I have locally which are a lot more fine grained.

Phil, there is plenty of good polish in that PR but it also adapts a few widespread things that we intentionally did differently: e.g. no copyright headers on package-info, no "this." prefix on logger access, no type+"this." prefix in inner classes for access to outer state... and allowing star imports in test classes.

I figured some of those were intentional, but I wasn't 100% sure. For Spring Boot I have a lot of IDE templates and auto-cleanup enabled so I've tended to take a blanket rule over exceptions. For example, if I create a new class file I get the copyright header, but Eclipse doesn't give me a way to skip it only for package-info.java files. Likewise, if I save a file in Spring Boot the IDE automatically adds this., but it doesn't offer a way to skip it for this.logger. Same with star imports, I either get all or none, so I opted for none.

I intentionally started this PR with the same approach to see what you thought about it. I think with checkstyle we can refine the rules to enforce what we want. I'll have a look to see if we can refine some of those checks to also enforce the exceptions. For me, one of the biggest benefits to checkstyle is helping to know what the rules are. I had no idea that this.logger shouldn't be used, so I think it would be really beneficial to enforce that one.

Generally speaking, I'm not sure whether enforcing production checkstyle guidelines on test sources is necessary or even desirable. At the very least, we could have more relaxed guidelines for test sources. For a start, enumerating the assertion method imports for every single test class feels odd to me... but I'm also generally much more relaxed about the code style used for test methods or inner classes used by them, with many production rules not needing to apply there.

It's easy to add a blanket exclude for src/test/* if that's a better option. I didn't do that for pretty much the same reason as above. Eclipse doesn't offer an easy opt-out of formatting and cleanup for src/test so I just treated them in exactly the same way.

As for single-argument lambdas, I visually object to the enforced parentheses there. Frankly, I'd rather wrap the expression itself in additional parentheses than such a plain parameter name part. We knew you chose differently in Boot but I'm afraid I'm not up for changing our style here in this respect.

No problem, I wasn't 100% sold on it myself at first in Boot (but I do really like the consistency now). I can refine the checkstyle rule in the other direction and enforce that all single parameter lambdas must not have parentheses.

The equals/hashCode stuff is unfortunately a common false signal by style-checking tools. All of those classes were valid since the superclass hashCode() implementation has a base hash that the subclasses choose to keep in that coarse-grained fashion. Enforcing hashCode() in those subclasses seems rather pointless, in particular when the overridden methods literally just call super and are effectively just there to keep checkstyle happy.

I agree, It's a little pointless. On the other hand, it found at least one that was genuine I think. Do you think the empty hashCode is worth it for those few false-positive cases, since it helps when new code is developed? Or would you rather drop that rule entirely?

With respect to consistent code blocks and consistent ternary expression style, there were a few glitches... but also a few cases where we intentionally used one-line if variants, e.g. in AbstractBeanDefinition for better readability of that long if cascade.

I could either drop the rule, or add an exception for the few cases where it looks better shortened. I'm personally in favor of hard rules here, just so nothing is open to interpretation. I think I use more brain cycles working out why a bit of code is formatted differently than just dealing with more whitespace.

I'm also not opposed to shortened blocks in test classes, and I don't see all those inverted ternary expressions as really worthwhile... when local readability doesn't really improve through that measure.

If we drop test checks then those blocks won't be enforced. On the other hand, if it's test code perhaps if doesn't really matter if they take up a bit more space? The ternary things is a bit strict as well. Stephane pointed out that you polish a lot of them to use != and he finds that it helps when scanning code because his eyes just go (the test ? the not equals case ? the equals case). Individually they don't add much, but collectively they might help. It will be easy to make that an option if you prefer?

All in all, from where I stand at the moment, the PR itself is way too much to merge. Let me rather go through the commits individually and hand-pick key changes while ignoring a whole range of others... and see where we end up with. In particular, I'm going to focus on production source fine-tuning, not touching test sources unless there is something totally bogus there. Once I've done an initial pass through this, let's see which checkstyle rules we'll eventually set up.

I'm happy to have another pass here as well if it helps. Splitting out the PR into test, production and unwinding a few of the rules discussed above shouldn't take me too long. Probably the most labor intensive part was the javadoc changes so I'd be super happy if those could go in?

Last but not least, thanks for your efforts there! This is pointing out a lot of stuff worth reviewing, even in areas where we might not enforce hard rules eventually.

Sorry it ballooned into such a monster. It really ended up being quite extreme all things considered!

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 23, 2018

Sam Brannen commented

3 things...

  1. Thanks for taking the initiative here.
  2. Yes, that is indeed quite a doozie of a PR. ;)
  3. Regarding the Eclipse formatter settings, I took a look, but TBH it's impossible to make heads or tails of that diff. Would you mind sorting the current settings on master alpha-numerically (and pushing to master) and then sort your updated settings the same way so that the diff becomes suitable for human consumption? That would make it much easier for me to make sense of the changes. Also, please change "5.0" to "5.1" for @since tags for new types.

Cheers!

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 25, 2018

Phil Webb commented

I'm doing another round on this one, I'll take the eclipse settings as different commit.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 26, 2018

Phil Webb commented

Sam Brannen The eclipse update is pushed

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 26, 2018

Phil Webb commented

Juergen Hoeller I've forced pushed a second round so the PR should be a bit easier to digest now. I know you wanted the javadoc one as a different PR but I've left it in this one for now (otherwise I can't get checkstyle to pass).

Here are the updates:

  • Tests are no longer verified and I've removed the test source from each commit. I've added a couple of commits for test polishing that I still think is worthwhile.
  • The javadoc commits has been squashed into a single commit. The @param rule on classes has been changed to match the existing style.
  • The copyright header is no longer applied to package-info.java and I've added a rule to enforce that nobody adds one by mistake.
  • I've kept the block style rule but changed {[AbstractBeanDefinition.equals}} in a polish commit to make it more palatable.
  • The this. rule is now enforced for most items but relaxed for inner classes. I've added a rule to make sure this.logger is never used.
  • I've kept the rule of equals must have hashcode but added exceptions for the override cases. There are still a few legitimate ones I think.
  • The lambda rules are now enforced in the other direction (all single arg lambdas must not use parenthesis)
  • Star imports have still been expanded in the src/main but we can do another round on this if you let me know the exceptions.
  • I've completely dropped the ternary checks for now. I'm not sure what the rules should be regarding parenthesis. I've see a & b ? "foo" : "bar", (a & b) ? "foo" : "bar" and (a & b ? "foo" : "bar"). If there's a rule that can be applied I can refine the checkstyle code.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 26, 2018

Sam Brannen commented

Phil Webb

The eclipse update is pushed

Where? To a different branch?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 26, 2018

Sam Brannen commented

Ahhh... you pushed to master. Found the commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task
Projects
None yet
Development

No branches or pull requests

2 participants