Fix issue #31 #45
Fix issue #31 #45
Conversation
| public boolean paddedCell = false; | ||
| @InputFiles | ||
| @OutputFiles |
There was a problem hiding this comment.
You will have to split the task into two classes:
- a
CheckFormattask - an
ApplyFormattask
Only the CheckFormat task can be up-to-date checked. The ApplyFormat task changes its own inputs, so it can't be cached/up-to-date checked. Also, if the ApplyFormat task declared the inputs as an output, then cleanApplyFormat would delete your source code ;)
| public Iterable<File> target; | ||
| @Input |
There was a problem hiding this comment.
When you have a List of complex objects as an input, these objects must:
- implement
Serializableso Gradle can write them to the cache - implement equals/hashcode, so Gradle can compare them to the previous value
There was a problem hiding this comment.
One idea (I think from @oehme in the issue discussion) is looking at the bytecode of the function, but I'm inclined to declare bankruptcy on input properties being clean w.r.t. incremental compilation. That leaves two options:
A) spotlessCheck will be wrong sometimes after you've changed your format
B) Have three tasks: spotlessCheck/spotlessApply and spotlessFastCheck. check/apply can use the same dumb logic from before, and fastCheck will be faster (using incremental stuff) but user beware that it might be wrong if you're using a custom function that you've changed.
I see Spotless' priorities as such:
- Be correct and repeatable.
- Make it easy to write a one-liner function to tweak a formatting issue.
- Be as fast as possible.
Based on these priorities, I'm inclined to go with option B. It lets us implement spotlessFastCheck without making a full correctness guarantee. We can add a section to the readme Speeding up Spotless which lists the spotlessCheckFast function and lists any provisos, e.g.
spotlessCheckFastwill be incorrect if you just changed the format rules. But if the rules have not changed, then it will be much faster than plain-oldspotlessCheck.
If we get it to the point that we don't need an asterisk around correctness, then we can dump the old spotlessCheck entirely and promote spotlessCheckFast to the sole check task.
There was a problem hiding this comment.
I'm pretty sure nobody would use a spotlessFastCheck if you have to run spotlessCheck to be sure it's correct anyway.
The only real alternative I see is properly supporting up-to-date checks. Instead of referencing Throwing.Function directly, we should reference a serializable sub-interface of it. Maybe call it FormatterFunction. Then implement the default functions (like regex) in such a way that they properly support equals/hashcode too.
The worst thing that could happen after that is that the task is out-of-date unnecessarily when users use inline steps. The readme about "Making spotless faster" could then explain that users should write real classes with equals/hashcode instead to make the task fast.
There was a problem hiding this comment.
I agree it's easy to implement all of Spotless' existing formatters in a serializable way. But I don't see an easy way to do this for custom user functions. #46 is a great example of why these custom functions are so useful, and why it's important for them to be easy.
We've got this custom function up-to-date wrinkle, and we can push it onto users in two ways:
A) We can make custom rule-writers deal with it
You can write a custom rule by implementing such-and-such class and then calling the constructor here, and if you change the rule be sure to change the serialVersionUID
B) We can make people who are optimizing their build deal with it
If you want to speed up spotlessCheck, you can fix it with spotlessFastCheck. Here are the provisos:
If somebody's code has formatting problems, it's probably not because running the formatter is too slow, but because it was hard to find or write a formatting rule that did what they want. So I don't think we should take a mild performance issue and turn it into a correctness/customizability issue.
I'm pretty sure nobody would use a spotlessFastCheck if you have to run spotlessCheck to be sure it's correct anyway.
It would still be very useful for --continuous. And imo, that's the only place this optimization makes sense anyway. spotlessCheck is much faster than even a very small unit test suite. And if we're able to remove this wrinkle later, then spotlessFastCheck is better than spotlessCheck in every way, and we can promote it to the job of sole-checker at that time.
There was a problem hiding this comment.
You can write a custom rule by implementing such-and-such class and then calling the constructor here, and if you change the rule be sure to change the serialVersionUID
I think you misunderstood my point above.
- if the author implements an inline rule it will always be out-of-date and thus correct
- if the author wants extra performance, they can write a class with equals/hashcode
This gives you the best of both worlds: Ease of use by default, performance on demand. No need for a 2-task solution.
There was a problem hiding this comment.
And imo, that's the only place this optimization makes sense anyway.
This is only true in very small projects. In a large project, when you change a single module, you want the other modules to be up-to-date, not spend time re-executing the formatter.
There was a problem hiding this comment.
Ahhh. Thanks for clarification, I misunderstood. In the approach you describe, a spotlessCheckFast would still be valuable for the --continuous case where you have custom rules which don't implement the full up-to-date contract.
But the simplicity of having only a single "check" class is very valuable, and it's a great feature for all tasks to always be correct. My gut is quite unhappy that Spotless' formatters are no longer just Function<String, String>, but I agree this would be a big speedup for large projects. You've convinced me :) I've got an idea for an easy and mostly-correct way to handle simple rules, which I've described in #47.
| problemFiles.add(file); | ||
| } | ||
| } | ||
| Errors.rethrow() |
There was a problem hiding this comment.
Can you give a short explanation why this wrapping needed now?
There was a problem hiding this comment.
It's to do with how, in Java 8, lambdas cannot throw checked exceptions unless the interface they target declares that it throws Throwable. IncrementalTaskInputs.outOfDate(Action) accepts a Groovy Action, which doesn't throw Throwable, so all checked exceptions thrown within the Action have to be wrapped in unchecked exceptions. Therefore, since !formatter.isClean(file) throws IOException - a checked exception - it has to be wrapped in an unchecked exception of sorts, otherwise it won't compile.
In this case, I decided to use Durian's Errors.rethrow() mechanism to wrap all IOExceptions as Errors.WrappedAsRuntimeExceptions, although I could have just as easily wrapped them in UncheckedIOExceptions instead (but that would arguably be less readable, as it would have required try { ... } catch(IOException e) { throw new UncheckedIOException(e); }).
There was a problem hiding this comment.
Sorry, that was a longer explanation than I expected to write. :)
There was a problem hiding this comment.
Thanks, makes perfect sense. Didn't know that method threw a checked exception. It could possibly be changed to use an unchecked exception further down.
| @@ -272,7 +272,7 @@ public void licenseHeaderFile(Object licenseHeaderFile, String delimiter) { | |||
| /** Sets up a FormatTask according to the values in this extension. */ | |||
| protected void setupTask(FormatTask task) throws Exception { | |||
| task.paddedCell = paddedCell; | |||
| task.lineEndingsPolicy = getLineEndingPolicy(); | |||
| task.lineEndingPolicy = getLineEndingPolicy(); | |||
There was a problem hiding this comment.
I don't care which name we use, but this is a public member of a public class documented in the README, so changing it breaks API compat for everyday buildscripts. These changes are likely going to require a major version bump anyway, but I'd rather not churn the lineEndingsPolicy name.
aside: I've enjoyed learning about a lot of Stream methods I didn't know about before, lots of good cleanup in various parts 👍
There was a problem hiding this comment.
I'm glad you're enjoying learning more about Stream methods. :D
|
I checked out the code and ran the tests, and it seems the mocks are not working properly. Easiest example is |
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class ApplyFormatTask extends DefaultTask { |
There was a problem hiding this comment.
This task and CheckFormat have quite some duplication, we should extract a base class. This base class can then also be used to get rid of the duplicated configuration logic in the plugin.
Will squash all commits at the end!
…guarantees correct up-to-date checks.
…asses which captured their parameters in non-transient fields. Thanks to FindBugs for spotting the error! New implementation ensures that the *only state* is the serializable key.
Note that in the case where the license header and delimiter are supplied as strings, it makes sense to store the state eagerly, whereas when the license header is supplied in a file, we load that file lazily, only if we actually need to.
… all non-strictified rules will always run.
… utility NonUpToDateCheckingTasks because they're still useful there.
|
I've pushed commit 4262b2b in an attempt to exclude build and cache dirs from the input files passed to Spotless. However, the tests no longer pass, and I think it's because I'm using @oehme Do you have any thoughts on where I might be going wrong? |
…e using the serialized form of the state. This means that formats which extend FormatExtension.Strict don't have to implement equals or hashCode.
|
These commits have all been merged into 3.x. I'm going to close this PR, and I've given you both write access to the repo. Feel free to push directly to 3.x if you'd like, but please leave master alone :) We can also continue to do the PR workflow, but it seems like there's gonna be a steady churn for a little while, so feel free to just push to 3.x if you prefer. |
|
Also, re: the current error in the filetree, here's the error message: |
|
Sorry, it seems I wasn't clear earlier. I actually did see that error message you linked in my own logs, but it's not clear to me from the message why it cannot convert the "provided notation into a File or URI". Is it because of the Ant-style paths e.g. If so, is there a way I can tell a fileTree to include the Gradle project folder, but exclude the |
|
Oh, and thank you very much for giving me write access to the Spotless repository! I'll do my best to maintain the trust you've put in me. 😄 |
|
Can you please provide the full stacktrace? |
|
Certainly! Here it is. |
|
The method takes a Why don't you use the fluent, staticallly typed API? fileTree(root).include(...).exclude(...) |
|
Thanks @oehme, that seems to work! I'll submit a fix to branch 3.x as soon as I can. |
No description provided.