-
Notifications
You must be signed in to change notification settings - Fork 460
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
Make padded cell mandatory #561
Conversation
…ters and setters.
…(), which now includes info about non-converging steps.
…to spotlessCheck/Apply.
This PR does two main things:
I think those two things are a slam dunk win. It's even more compelling because it makes enabling the buildcache much easier. If there's going to be any contention, it is probably over this
This is the API I chose (with sentinel values for the isClean/didNotConverge cases). spotless/lib/src/main/java/com/diffplug/spotless/PaddedCell.java Lines 225 to 250 in 9bfdc90
Open to any feedback! One of the main goals of this PR is to make #560 easier to do, so there are some changes that seem arbitrary, but should plop into place for #560. |
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.
Hi @nedtwigg, I've been following your ongoing conversation with @bigdaz in this repository on improving the performance (and maintainability) of Spotless further, which has excited me, so thanks for involving me in this!
In the README.md
(in a place where GitHub's UI doesn't allow me to review it directly), there is a paragraph that says,
Spotless now has a special
paddedCell()
mode. If you add it to your format as such:spotless { format 'cpp', { ... paddedCell() } }then it will run in the following way:
I think it should now be simplified to something like this:
Spotless now has a special "padded cell" mode, which is always enabled and runs in the following way:
I've added some more review comments below.
I've only had time to review part of this PR, so I'll let you know if I get around to looking at the rest. :)
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've just gone through the rest of this PR, and other than my earlier comments, it looks good to me! 👍
- byte[] canonicalBytes() is now private, and IllegalStateException if called incorrectly - access to canonicalBytes() is handled by `void writeCanonicalTo(File file)` and `void writeCanonicalTo(OutputStream out)`
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.
Thanks for addressing my earlier comments! The README.md
looks very good now.
I've just found one more place that could be improved, but I don't want to hold up this PR for too long, so I'll mark my review as "approved", and feel free to address my comment below at your own pace.
@@ -28,6 +29,8 @@ | |||
import java.util.Objects; | |||
import java.util.function.Function; | |||
|
|||
import org.omg.CORBA.portable.OutputStream; |
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.
Oops, did you mean to import java.io.OutputStream
rather than org.omg.CORBA.portable.OutputStream
here?
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.
Lol, thanks, good catch!!! The dangers of not following yagni ;-)
It simplifies the code quite a bit to just always do it, and it turns out that we were already doing enough of PaddedCell that it isn't slower anyway.