Simple spike at separating the core SpotlessTask from check and apply#560
Simple spike at separating the core SpotlessTask from check and apply#560bigdaz wants to merge 5 commits intodiffplug:masterfrom bigdaz:incremental_spike
SpotlessTask from check and apply#560Conversation
- SpotlessTask runs the formatter for each source file, and generates an
output file with the formatted results (if different)
- If the task is not incremental, all prior outputs are removed
- For each added file, the formatter is run, and the output is
written if different from the original
- For each removed file, the previous output is removed, if present
- SpotlessCheck will fail if there are any outputs from SpotlessTask
- SpotlessApply will copy any formatted outputs back over the original source
This mechanism has a number of benefits and makes things a lot simpler:
- The spotless cache is not required
- SpotlessTask is now a simple incremental task and benefits from the
usual up-to-date checking. It should also support being made cacheable.
Note that a number of things were ignored for this spike:
- Anything to do with `PaddedCell`
- Check does not print out the actual formatting issues
- I didn't run any of the tests
|
@nedtwigg I spent some time experimenting with a solution to the cacheability of Spotless in Gradle. This is far from a finished solution, but demonstrates how I think it could work. In the end it's a lot simpler because there are no tasks that declare the same values as input and output. Please take a look and let me know if you'd be interested in a more complete solution following along this vein. Thanks. |
|
Looks great, thanks very much! Unless we hit a showstopper (I don't see any), this will definitely get merged as we work through any issues, I definitely agree that it simplifies things. I'll take a deeper look later and leave comments and perhaps push a commit or two - there are a few things that pop out where I think I can simultaneously turn the unit tests green and make the total diff a little smaller. I guarantee a turnaround by tomorrow (Thurs) am. |
|
Thanks Ned. This is definitely spike code, but any feedback/improvements are welcome. |
|
I wanted to dig in to the padded cell stuff, because I had a vague recollection of tech debt there. The story is:
Looking at it with fresh eyes, we can leave
A question I have for you is: it doesn't make sense to send non-formatted |
|
More follow-up - this PR is much easier if PaddedCell is just always on. The refactor is easier/safer if you remove PaddedCell on its own, and then do the task restructuring that you've outlined here. I tried to do both at once, but I'm uncomfortable with how many tests are turning red for different unrelated reasons - we've got hard-earned tests for subtle gotchas in PaddedCell and in up-to-date checking, I don't want to accidentally throw any out by conflating them. I'm hoping to push up the PaddedCell PR today, and once that's merged we can take a second pass on this PR. |
|
OK, thanks Ned. So as I understand it, you're going to push a PR so that PaddedCell is always enabled. After that I can take another pass at this PR, this time in a more rigorous fashion. |
|
Perfect! |
|
@bigdaz okay! |
2 participants
output file with the formatted results (if different)
written if different from the original
This mechanism has a number of benefits and makes things a lot simpler:
usual up-to-date checking. It should also support being made cacheable.
Note that a number of things were ignored for this spike:
PaddedCell