-
Notifications
You must be signed in to change notification settings - Fork 459
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
Serializable Groovy, Java, Kotlin, and Scala steps #2033
Conversation
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.
Amazing @Goooler, thanks so much! I'm gonna pull this locally, do some testing, and then merge. Sorry it sat for so long.
|
||
import java.io.Serializable; | ||
|
||
public interface RoundedStep extends Serializable {} |
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.
The intent here is to be a marker interface?
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.
Yeah.
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.
@Goooler My YAGNI senses are tingling! I'm wondering if you can clarify why this interface was introduced, and if we can get away with inlining Serializable
instead?
Otherwise, this all LGTM. 👍
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 was thinking that we can do some checks based on a common step type instead of names here
} else if (Set.of("black", "buf", "clang", "ktlint", "ktfmt", "scalafmt", "palantir-java-format", "google-java-format", | |
"removeUnusedImports", "cleanthat", "No line break between type annotation and type", | |
"importOrder", "Remove unnecessary semicolons").contains(onlyStepName)) { | |
supportsRoundTrip = true; |
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.
That's a good idea, but it's hard to get to that state. I figured it was probably meant as a testing thing, we can remove it when we finish #1274
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.
Sounds good to me either way! 👍
I added some comments in If you run the first commit, you'll see the tests fail, and then pass in the second commit. Wanna push those commits into this PR and then I'll press merge? |
Thanks for your feedback! |
…rStep` serializable.
# Conflicts: # testlib/src/main/java/com/diffplug/spotless/StepHarnessBase.java
Refs #1274 (comment).