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 and javaformat plugins #16

Closed

Conversation

onobc
Copy link
Collaborator

@onobc onobc commented Dec 29, 2023

Points of Interest

  • I DID run ./gradlew format on commit 2
  • I did NOT fix all of the checkstyles as there are MANY MANY. I DID set them to ignoreFailures = true but the report still goes out when we build so as to not fall into the "out of sight, out of mind" trap.

TODO

  • The checkstyle config files were lifted from Spring Pulsar which in turn lifted from Spring Boot and minimized a bit. They have worked well for Spring Pulsar but we can take a scan through them and make sure we all agree w/ them.

COMMITTER REQUESTS

I have manicured the commits into the 2 divisible units that I think make sense to have in the repo for history

  1. add the plugins
  2. apply the format plugin
    Please and thank you.

@onobc onobc requested review from corneil and artembilan December 29, 2023 19:07
@onobc onobc force-pushed the GH-7-add-javaformat-and-checkstyle branch from 19b637a to bc6b809 Compare December 30, 2023 03:12
@onobc onobc force-pushed the GH-7-add-javaformat-and-checkstyle branch from c87aa3b to 819a542 Compare January 2, 2024 21:57
<module name="io.spring.javaformat.checkstyle.SpringChecks">
<property name="excludes" value="io.spring.javaformat.checkstyle.check.SpringHeaderCheck" />
<property name="excludes" value="com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocPackageCheck" />
<property name="excludes" value="com.puppycrawl.tools.checkstyle.checks.javadoc.MissingJavadocMethodCheck" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the failure in PR build we also need add this exclusion yet: com.puppycrawl.tools.checkstyle.checks.javadoc.MissingJavadocTypeCheck

apply from: "${rootDir}/publish-maven.gradle"
apply from: "${rootDir}/gradle/checkstyle-conventions.gradle"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since our checkstyle-conventions.gradle is already short enough I don't see a reason in the separate file.
I also wonder why it still fails even if we have that ignoreFailures = true...

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's too much, Chris, I can merge this and we can investigate in the separate issues/PRs.
Thanks

<property name="headerFile" value="${config_loc}/checkstyle-header.txt"/>
<property name="fileExtensions" value="java"/>
<module name="com.puppycrawl.tools.checkstyle.checks.header.RegexpHeaderCheck">
<property name="headerFile" value="${config_loc}/checkstyle-header.txt" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this issue: spring-io/spring-javaformat#303, our file must be this: https://github.com/spring-io/spring-javaformat/blob/main/spring-javaformat/spring-javaformat-checkstyle/src/main/resources/io/spring/javaformat/checkstyle/check/header-apache2.txt.
I wonder why it is not by default there in SpringChecks...
So, we would not need to manage that file, but rather rely in whatever Javaformat provides for us 🤷

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it does that: spring-io/spring-javaformat#70.

So, never mind: merging and moving on with that locally.

Will let you know with a fresh PR.

Thanks

@artembilan artembilan enabled auto-merge (squash) January 3, 2024 15:36
@artembilan
Copy link
Member

Merged as 836708f

@artembilan artembilan closed this Jan 3, 2024
auto-merge was automatically disabled January 3, 2024 15:41

Pull request was closed

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

Successfully merging this pull request may close these issues.

2 participants