-
Notifications
You must be signed in to change notification settings - Fork 267
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
Adding Checkstyle linter #2688
Adding Checkstyle linter #2688
Conversation
d205a84
to
55b2776
Compare
ea6e830
to
46c3fec
Compare
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.
Great job, the spotless addition seems a very nice one. I think that these tasks will be very useful to make the code follow a pattern. 👏
I have a few comments regarding the date, once we clear that out, I will approve.
Hey guys, a couple of months ago I worked on something similar. Leaving PR here so you can check it out. This is a great integration! |
rskj-core/build.gradle
Outdated
import java.nio.file.Paths | ||
|
||
// Define static timestamp | ||
def staticTimestamp = '2024-09-03T02:17:00' |
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.
There is also the option to use ratchetFrom, which only formats from a given git tag
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 the suggestion! I considered using ratchetFrom
to format only changes from a specific git tag. However, the last tag in the repository is from July 10, which is quite a while ago. Using ratchetFrom would include all changes made since that tag, which would affect files modified after that date—something we want to avoid in this case
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 see, thanks! Do you think it makes sense to switch to ratchetFrom
once a new git tag is released?
5f73a86
to
50e8d7e
Compare
f650cee
to
90b67b5
Compare
218c605
to
67cc1f6
Compare
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.
Looks good to me :) Added some minor comments for your consideration
rskj-core/build.gradle
Outdated
indentWithTabs(2) | ||
indentWithSpaces(4) |
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.
Should we choose only one, either indent with tabs or indent with spaces? How will it choose how to do the indentation in this case?
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.
you're right! chose indentWithTabs(2) and removed indentWithSpaces(4). Visually, the result was the same with both
6c296bf
to
26da192
Compare
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
Quality Gate passedIssues Measures |
Add Checkstyle with Google Code Conventions and Spotless for Google Java Code Formatting
depends on powpeg pr: https://github.com/rsksmart/powpeg-node/pull/324/files
Description
This PR introduces Checkstyle 8.45, configured with Google Code Conventions, and adds Spotless to our project for code formatting using the official Google Java Format. Checkstyle will analyze the codebase to enforce coding standards, while Spotless ensures consistent code formatting. This addition aims to improve code quality and maintain consistency for new contributions.
Due to security concerns with the previous unofficial Google Java Formatter plugin by sherter, which was from a less-established source, Spotless was chosen as a more widely used and trusted alternative. Spotless not only integrates Google's official format but also offers better reliability and security for formatting our code.
Highlights:
Motivation and Context
This update is designed to help our project follow the latest best practices and industry standards for Java-based open-source projects. By adding Checkstyle with Google Code Conventions and the Google Java Formatter, we’re making sure that all new code meets high-quality standards and maintains consistent formatting.
Basic Implementation:
Why Use Spotless with Official Google Java Formatter
I have chosen Spotless with the official Google Java Format over the unofficial
sherter
plugin due to several reasons:sherter
plugin comes from a less-established source, raising potential security concerns.File Checking Mechanism: The method for filtering files has been updated. Previously, Checkstyle and the Google Formatter relied on the
lastModified()
timestamp of files, which could lead to inconsistencies. For example, if you modify file A and then undo those changes, thelastModified()
timestamp might still indicate that the file was recently changed, causing Checkstyle and the formatter to run on it even though the modifications were reverted.To address this, we now use Git commands to identify modified, staged, and committed files based on a specific timestamp. This method ensures that Checkstyle and Spotless formatting checks are applied more accurately. For instance, with the new approach, if you revert changes to file A, Git will no longer include it in the list of modified files, so neither Checkstyle nor Spotless will check it unnecessarily. This ensures more precise and reliable code quality and formatting checks.
To run Checkstyle and verify the new code, use the following command:
./gradlew clean rskj-core:checkstyleMain
To verify files to format without formatting them, use the following command:
./gradlew clean rskj-core:spotlessCheck
To format code, use the following command:
./gradlew clean rskj-core:spotlessApply
Example of Suppression:
In
suppressions.xml
, you can suppress a rule like this:This example suppresses a specific Checkstyle rule for
SomeFile.java
. The suppressions file helps tailor the Checkstyle configuration to fit our project needs more precisely.Example of the Formatting Mechanism:
The file filtering mechanism now uses Git commands to determine which files have been modified, staged, or committed since a specific timestamp. This ensures that only the relevant files are processed, based on actual changes tracked by Git.
Example:
Modified File (
/rskj/rskj-core/src/main/java/co/rsk/config/GasLimitConfig.java
):- Before Formatting:
- After Formatting:
And then, if we run Checkstyle, the console might look something like this:
fed:updating_dependencies_for_spotless