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

Simple spike at separating the core SpotlessTask from check and apply #560

Closed
wants to merge 5 commits into from
Closed

Simple spike at separating the core SpotlessTask from check and apply #560

wants to merge 5 commits into from

Conversation

bigdaz
Copy link
Contributor

@bigdaz bigdaz commented Apr 29, 2020

  • 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

- 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
@bigdaz
Copy link
Contributor Author

bigdaz commented Apr 29, 2020

@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.

@bigdaz bigdaz marked this pull request as draft April 29, 2020 23:18
@nedtwigg
Copy link
Member

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.

@bigdaz
Copy link
Contributor Author

bigdaz commented Apr 30, 2020

Thanks Ned. This is definitely spike code, but any feedback/improvements are welcome.
I've tried not to use any "modern" Gradle features (e.g. Provider) but I didn't verify that they didn't sneak in.

@nedtwigg
Copy link
Member

I wanted to dig in to the padded cell stuff, because I had a vague recollection of tech debt there. The story is:

  • users expect spotless apply to be idempotent, but lots of formatters have edge cases
  • we added paddedCell mode to fix them
  • we added a way to detect if you don't need paddedCell anymore, since it was slower
  • it turned out that people only need paddedCell for a single run usually. That one run would get them out of the buggy-zone of the formatter, and they didn't need it anymore
  • so we tried to accomodate that common-case automatically, and ended up with paddedCell smeared all over everything

Looking at it with fresh eyes, we can leave paddedCell on at all times without any penalty performance compared to the smeared-thing we ended up doing, and that simplifies things greatly. There are a few things I'd like to assign myself in this PR:

  • fix the tests
  • cleanup our paddedCell mess

A question I have for you is: it doesn't make sense to send non-formatted spotlessApply results to buildcache, imo. Can we use cacheIf to disable buildcache if the output directory isn't empty? Is that crazy?

@nedtwigg
Copy link
Member

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.

@nedtwigg nedtwigg closed this Apr 30, 2020
@bigdaz
Copy link
Contributor Author

bigdaz commented May 1, 2020

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.

@nedtwigg
Copy link
Member

nedtwigg commented May 1, 2020

Perfect!

@nedtwigg
Copy link
Member

nedtwigg commented May 3, 2020

@bigdaz okay! master is now ready. Should be a lot easier this time around :)

@nedtwigg nedtwigg mentioned this pull request May 4, 2020
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.

2 participants