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 config #14

Merged
merged 3 commits into from
Jul 4, 2023
Merged

Add checkstyle config #14

merged 3 commits into from
Jul 4, 2023

Conversation

haocheng
Copy link

@haocheng haocheng commented Jul 3, 2023

Motivation:

  • Add Checkstyle config to make sure we are following the same style convention

Modifications:

  • Add config in build.gradle to run checkstyle
  • Add checkstyle rules configurations
  • Fix checkstyle issues in the current code base
  • Use pull_request_target to workaround Github Action permission issue

Result:

* Add Checkstyle config to make sure we are following the
  same style convention
* Fix checkstyle issues in the current code base
@haocheng haocheng force-pushed the feat/add-checkstyle branch from 5b993e3 to 5622b28 Compare July 3, 2023 09:37
Comment on lines +43 to +48
- name: Upload checkstyle report
uses: actions/upload-artifact@v3
if: success() || failure()
with:
name: checkstyle-report
path: build/checkstyle/*.html
Copy link
Author

@haocheng haocheng Jul 3, 2023

Choose a reason for hiding this comment

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

I would like to display checkstyle report by using Push Checkstyle Report action but failed due to the Error: HttpError: Resource not accessible by integration issue...

configProperties = ['checkstyleConfigDir': "$checkstyleConfigDir"]
ignoreFailures = true
reportsDir = project.file('build/checkstyle')
toolVersion = '9.3'
Copy link
Author

Choose a reason for hiding this comment

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

The latest version to support Java 8 is 9.x (10.x only supports Java 11)

@@ -17,12 +17,14 @@ jobs:
java-version: ${{ matrix.java }}
architecture: x64
- uses: actions/checkout@v3

with:
ref: ${{ github.event.pull_request.head.sha }}
Copy link
Author

Choose a reason for hiding this comment

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

Used ${{ github.event.pull_request.head.sha }} according to the suggestion on Push Checkstyle Report
Screenshot 2023-07-04 at 9 39 06

"-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
"https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
Copy link
Author

Choose a reason for hiding this comment

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

I have copied this file from Armeria but removed checks related to Javadoc:

    <module name="JavadocType">
      <property name="scope" value="protected"/>
      <property name="excludeScope" value="package"/>
    </module>
    <module name="JavadocMethod">
      <property name="accessModifiers" value="protected, public"/>
      <property name="allowMissingParamTags" value="true"/>
      <property name="allowMissingReturnTag" value="true"/>
      <property name="allowedAnnotations" value="Override, Test"/>
    </module>
    <module name="MissingJavadocMethod">
      <property name="scope" value="protected"/>
    </module>
    <module name="MissingJavadocPackage"/>
    <module name="MissingJavadocType"/>
    <module name="JavadocParagraph"/>
    <module name="JavadocTagContinuationIndentation"/>

@haocheng haocheng marked this pull request as ready for review July 4, 2023 00:42
@haocheng haocheng requested review from kojilin and ikhoon July 4, 2023 00:43
@kojilin kojilin merged commit 7b50f45 into line:master Jul 4, 2023
@haocheng haocheng deleted the feat/add-checkstyle branch July 4, 2023 23:38
@haocheng haocheng added this to the 0.5.0 milestone Jul 5, 2023
@haocheng haocheng added the CI github action related label Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI github action related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Checkstyle config
2 participants