Skip to content

Apply spotless after quick-check failures#3551

Open
niloc132 wants to merge 5 commits into
deephaven:mainfrom
niloc132:quick-apply-spotless
Open

Apply spotless after quick-check failures#3551
niloc132 wants to merge 5 commits into
deephaven:mainfrom
niloc132:quick-apply-spotless

Conversation

@niloc132

Copy link
Copy Markdown
Member

Automatically applies spotless after a quick check fails, and presents the diff as github PR comment suggestions for the author to fix.

Example pull request showing this in action (with the suggestion manually "unresolved" so that it is visible when looking at the changes): niloc132#4

image

Comment on lines +58 to +62
- name: suggester / spotless
uses: reviewdog/action-suggester@v1
if: failure()
with:
tool_name: spotless

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Neat tool! I'm assuming if the job fails for other reasons, and there is no diff, the tool won't suggest any changes?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I mean it might? It depends on if the spotlessjavaapply actually made any changes...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I imagine this must be the case - but I want to make sure that if there are no spotless changes, then the suggester doesn't make an "empty" suggestion or something like that.

if: failure()
with:
job-id: spotless-formatting
arguments: spotlessJavaApply

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any reason to prefer spotlessJavaApply instead of spotlessApply?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah good call, that's better. I'm also adding updateProto to this as a separate step, so the changes are marked separately.

@mofojed mofojed left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any reason not to merge this?

Side note - I prefer to format on save, any reason we don't on Community?

@devinrsmith

Copy link
Copy Markdown
Member

Side note - I prefer to format on save, any reason we don't on Community?

@mofojed what do you mean "format on save"?

@niloc132

niloc132 commented Apr 4, 2023

Copy link
Copy Markdown
Member Author

IntelliJ can format on save, but its formatting tools don't quite agree with spotless, so you end up going back and forth if you dont have it perfectly configured.

I'm holding off trying to get the proto stuff right too, but I can probably back that out and just merge as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants