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

PF-1924: Add an action to run spotlessApply #310

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yuhuyoyo
Copy link
Contributor

No description provided.

.github/workflows/tests-nightly.yml Outdated Show resolved Hide resolved
.github/actions/spotless-apply/action.yml Outdated Show resolved Hide resolved
@yuhuyoyo yuhuyoyo marked this pull request as ready for review August 24, 2022 12:59
Copy link
Contributor

@melissachang melissachang left a comment

Choose a reason for hiding this comment

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

I don't understand the problem. Our repo is configured so that you can't merge if spotlessCheck fails.

First, lint-and-static-analysis runs ./gradlew spotlessCheck:

image

Second, the run test task runs spotlessApply:

image

@yuhuyoyo
Copy link
Contributor Author

for some reason it has broken before. Dex may be able to speak more about why but he recently had to manually do a spotlessapply and that PR has hundreds of files.

@melissachang
Copy link
Contributor

I still have the questions I asked on Aug 24. @dexamundsen , how did you generate your formatting PR?

I also have the same questions that I had for #1938. This GHA is highly unusual. Usually for a formatting PR:

  1. Someone runs formatting command
  2. Someone sends PR with just output from formatting command

The output of the normal process and the output of this action (the resulting PR) are the same. There are many kids of PRs where you do one thing, then you send out a PR for review. We don't create actions for all those things. I don't understand the benefit of this action.

@yuhuyoyo
Copy link
Contributor Author

I still have the questions I asked on Aug 24. @dexamundsen , how did you generate your formatting PR?

#301
this is the PR I'm referring to.

I also have the same questions that I had for #1938. This GHA is highly unusual. Usually for a formatting PR:

  1. Someone runs formatting command
  2. Someone sends PR with just output from formatting command

The output of the normal process and the output of this action (the resulting PR) are the same. There are many kids of PRs where you do one thing, then you send out a PR for review. We don't create actions for all those things. I don't understand the benefit of this action.
maybe they all should be automated. My principle here is that if it's not reviewable, it shouldn't need to get reviewed. And a human created PR should be reviewed.

I didn't look at the 186 files to stamp it. So this kind of PR that can be done automatically shouldn't require human action. i could accidentally get some exploration changes into this PR and no one would notice it (186 files!) Google has those bots that generate PR to clean up dead code, etc. This is not a frequent thing. If spotlessCheck passes, nothing will be done.

@dexamundsen
Copy link
Contributor

Apologies for the delayed response - on PR #301 -> I set the TDE to use google formatting and applied formatting manually (right click, format). Since the gradle build (both local and as part of PR) did not change any further files, did not second guess the IDE applied formatting

@melissachang
Copy link
Contributor

Apologies for the delayed response - on PR #301 -> I set the TDE to use google formatting and applied formatting manually (right click, format). Since the gradle build (both local and as part of PR) did not change any further files, did not second guess the IDE applied formatting

Ah, that's not the usual way we do formatting. As I said on Aug 24, we run spotlessApply/spotlessCheck on all code before merging. We don't need an action to run spotlessApply. It would do nothing, since spotlessApply was already run on the code.

FYI - google-java-format produces slightly different results from spotlessApply. If you want Intellij to run spotlessApply instead of google-java-format, here's how.

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.

3 participants