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-1559] fix spotless & run 🏃🏻 #676

Merged
merged 4 commits into from
Jun 9, 2022
Merged

Conversation

jaycarlton
Copy link
Contributor

Remove an over-optimization that made spotlessApply and its ilk rarely actually run. The first file is the only manual change.

@jaycarlton jaycarlton requested a review from ddietterich June 7, 2022 21:25
@jaycarlton
Copy link
Contributor Author

Testing the GHA with an intentional lint failure.

@ddietterich
Copy link
Contributor

Can we make an entry in the spotless config here to skip the generated code?

@jaycarlton
Copy link
Contributor Author

jaycarlton commented Jun 8, 2022

Can we make an entry in the spotless config here to skip the generated code?

Great idea. That looks like the ticket.

I'm stuck again though, because I don't understand why that annotation doesn't apply in this case for spotlessJavaCheck. 😕

@jaycarlton jaycarlton marked this pull request as ready for review June 8, 2022 21:10
@jaycarlton jaycarlton requested a review from yuhuyoyo June 8, 2022 21:10
Copy link
Contributor

@ddietterich ddietterich left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1,5 +1,10 @@
package bio.terra.workspace.service.resource.controlled.cloud.azure.storage;

import static org.hamcrest.MatcherAssert.assertThat;
Copy link
Contributor

Choose a reason for hiding this comment

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

@jaycarlton This import ordering doesn't seem to be enforced by google-java-format, as when I reformat wit google-java-format in IntelliJ I don't get these changes. Therefore it seems that I need to run spotless locally also. What is the command to do that, and do you know if there is a way to set up IntelliJ's formatting to automatically use spotless?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To run spotless, do ./gradlew spotlessApply after this is merged.

It's possible to set up IntelliJ so that it runs spotlessApply every time a file is saved. @melissachang is the only one I know who's done that.

Also, import ordering seems to work for me when I let IJ handle all the imports. I.e. I never manually add an import statement, and they seem to go to the right place FWIW.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have Intellij run spotless on file save:

diffplug/spotless#200 (comment)

@@ -12,11 +12,6 @@ for (task in TASKS_ONLY_IF_COMPILE_JAVA) {
task.dependsOn(compileJava.inputs.files)
// If compileJava runs, this must run after it (for the following to work).
task.mustRunAfter(compileJava)
// Don't run if compileJava didn't do any work. This is necessary because otherwise, these
Copy link
Contributor

Choose a reason for hiding this comment

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

is this related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, one fix was to make spotlessApply, etc, actually run, and the other was to avoid running it on generated code.

@jaycarlton jaycarlton merged commit 06d2a64 into main Jun 9, 2022
@jaycarlton jaycarlton deleted the jaycarlton-PF-1559 branch June 9, 2022 14:18
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.

5 participants