feat: ProtoCommentLinter and Spotless Custom Step #108
Conversation
Signed-off-by: Thomas Moran <152873392+thomas-swirlds-labs@users.noreply.github.com>
thomas-swirlds-labs
changed the title
There was a problem hiding this comment.
I left some comments. We need to make sure this is working as expected, before it can be merged.
You can add the plugin to the list of plugins in org.hiero.gradle.module.library.gradle.kts
plugins {
...
id("org.hiero.gradle.check.spotless-protobuf")
...
}
Then the check is automatically integrated/active when there are proto files in a module.
You can test it now against this branch of hedera-services:
hiero-ledger/hiero-consensus-node#17737
|
|
||
| spotless { | ||
| format("proto") { | ||
| target("src/main/proto/**/*.proto") |
There was a problem hiding this comment.
Note: we would need to make sure that this matches all the locations proto files are located in.
In hedera-services/hapi, There they are not located in src/main/proto. I think the solution is to move the files there. It would be more consistent.
This is also helpful for publishing the protobufs directly from the services repo (hiero-ledger/hiero-consensus-node#17425).
I started on this: hiero-ledger/hiero-consensus-node#17737
You can test this PR in hedera-services against the Branch of hiero-ledger/hiero-consensus-node#17737
| return issues; | ||
| } | ||
|
|
||
| public static String lintAndFix(String protoContent) { |
There was a problem hiding this comment.
This method is never called. How is this intended to work in the end? Are you able to auto-format some things but others need to be fixed manually?
There was a problem hiding this comment.
The first phase of this will be just to get a check working in a minimalistic way that at least informs the developer that there are styling rules that are not being followed and suggests remediations. This code is not needed yet so I will remove 👍
| } | ||
|
|
||
| if (!issues.isEmpty()) { | ||
| throw new RuntimeException("Proto file linting failed:\n" + String.join("\n", issues)); |
There was a problem hiding this comment.
When I test this, all the newlines are swallowed. Spotless has a specific features for Lint errors now.
| throw new RuntimeException("Proto file linting failed:\n" + String.join("\n", issues)); | |
| throw Lint.shortcut(issues); |
|
|
||
| public static String lint(String protoContent) { | ||
| List<String> lines = Arrays.asList(protoContent.split("\n")); | ||
| List<String> issues = new ArrayList<>(); |
There was a problem hiding this comment.
Would be best to use com.diffplug.spotless.Lint
| List<String> issues = new ArrayList<>(); | |
| List<Lint> issues = new ArrayList<>(); |
| String line = lines.get(i).trim(); | ||
|
|
||
| if (INLINE_COMMENT_PATTERN.matcher(line).matches()) { | ||
| issues.add("⚠️ Warning: Inline comment detected at line " + (i + 1) + ". Use `/** ... */` instead."); |
There was a problem hiding this comment.
| issues.add("⚠️ Warning: Inline comment detected at line " + (i + 1) + ". Use `/** ... */` instead."); | |
| issues.add(Lint.atLine((i + 1), "comment", "⚠️ Warning: Inline comment detected at line " + (i + 1) + ". Use `/** ... */` instead.")); |
Other places doing issues.add need to be adjusted accordingly.
| plugins { id("com.diffplug.spotless") } | ||
|
|
||
| spotless { | ||
| format("proto") { |
There was a problem hiding this comment.
There would be the option to combine your custom formatting with the "buf" formatter, for which there is Spotless integration:
https://github.com/diffplug/spotless/blob/main/plugin-gradle/README.md#protobuf
But IIRC you already said that's not needed right?
There was a problem hiding this comment.
Yes we can integrate the buf formatter for syntax and structural standards such as license header, etc. And that should be relatively straightforward. This PR was intended to address standards and recommendations for comments in the .proto files, however - so I was thinking we may want to keep them separate to simplify things for now 🤔
There was a problem hiding this comment.
Yes. Just wanted to mention it to make sure we explore the possibility. We can do it in a separate step. I added a comment here: #107 (comment)
| spotless { | ||
| format("proto") { | ||
| target("src/main/proto/**/*.proto") | ||
| addStep(org.hiero.gradle.spotless.ProtoLintStep.create()) |
There was a problem hiding this comment.
Should probably also add license header check. It would look like this:
licenseHeader(LicenseHeader.javaFormat(project), "(syntax)").updateYearWithLatest(true)
Which would put the header above the syntax keyword and other comments would then have to go below it.
But if I look at the "proto" files in hedera-services, the license header is not in the top right now but somewhere in the middle. Does not look right....
ProtoLintSpotless FormatterStepProtoCommentLinterimplementation for initial linting and recommendations on comment specificationsFixes #107