-
Notifications
You must be signed in to change notification settings - Fork 63
Enable checkstyle on autoconfigure/docs modules #226
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
Conversation
.../main/java/org/springframework/grpc/autoconfigure/client/ConditionalOnGrpcClientEnabled.java
Show resolved
Hide resolved
| <configprops.path>${project.basedir}/src/main/antora/modules/ROOT/partials/_configprops.adoc</configprops.path> | ||
| <configprops.inclusionPattern>spring.grpc.*</configprops.inclusionPattern> | ||
| <jruby.version>9.4.6.0</jruby.version> | ||
| <disable.checkstyle.checks>true</disable.checkstyle.checks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be this that causes the build failure. I don't see why this should be a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can get this in after the release.
...ain/java/org/springframework/grpc/autoconfigure/client/GrpcChannelFactoryConfigurations.java
Outdated
Show resolved
Hide resolved
...va/org/springframework/grpc/autoconfigure/server/security/GrpcSecurityAutoConfiguration.java
Show resolved
Hide resolved
onobc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for tackling this one @therepanic . I have left a couple of comments that should account for the test failures. That being said, I need more time to digest the large changes in the auto-configuations (I always am extra careful w/ these changes as they can wreck havoc on things in a very subtle way).
Can you describe why the checkstyle required such large refactoring in this area? Also, remember that if we want to make checkstyle ignore sections we can do ...
//CHECKSTYLE:OFF
var myBlockOfCodeThatIDontWantToDealWithRightNow = true;
....
//CHECKSTYLE:ONOnce we get this to a happy state I think we should wait to merge this until after the 0.9.0 release. @dsyer wdyt?
6c3c883 to
054f2bc
Compare
|
Thank you for your review. As you can see, most of the changes were made to the order of imports, the order of methods, and so on. Also, the order in javadoc. All of this accumulated over time, which is why so many changes had to be made. I force-pushed because I accidentally touched pom.xml in the test module, which is not related to these changes. I also added a commit that fixes your comment. However, that test still fails. |
...ain/java/org/springframework/grpc/autoconfigure/client/GrpcChannelFactoryConfigurations.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Andrey Litvitski <andrey1010102008@gmail.com>
Signed-off-by: Andrey Litvitski <andrey1010102008@gmail.com>
Signed-off-by: Andrey Litvitski <andrey1010102008@gmail.com>
Signed-off-by: Andrey Litvitski <andrey1010102008@gmail.com>
Signed-off-by: Andrey Litvitski <andrey1010102008@gmail.com>
17279f0 to
67525b4
Compare
|
I had to do a rebase because after all the fixes, the code still failing due to f90cecc. In the last commit, I made all the corrections. I also disabled (at least temporarily) checkstyle on the doc module. I looked at what corrections needed to be made for checkstyle to approve the doc module and changed my mind. In order for checkstyle to pass in the doc module, we need to write an empty constructor in What do you think about this? It seems to me that this is the very class for which we do not need to have an empty constructor. In any case, if we do checkstyle, we will also have to replace |
onobc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me. Thank you for tackling this @therepanic !
The motivation to do this came from @onobc's comment at #223 (comment). I think it would be worthwhile for us to include checkstyle at least in the autoconfigure module.
There were classes such as
GrpcSecurityAutoConfigurationandOAuth2ResourceServerAutoConfigurationthat contained non-static classes, which should not be done. In this case, I split them into new classes so that checkstyle would pass.Also, the
GrpcClientAutoConfigurationTeststest is currently failing. I would like to ask if anyone has any ideas as to why this is happening.