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

Separate the core SpotlessTask from check and apply #576

Merged
merged 15 commits into from
May 14, 2020
Merged

Separate the core SpotlessTask from check and apply #576

merged 15 commits into from
May 14, 2020

Conversation

bigdaz
Copy link
Contributor

@bigdaz bigdaz commented May 13, 2020

With this change, the Spotless Gradle plugin will leverage more of the
core Gradle functionality for incremental builds, thus avoiding some
of the complication present in the previous implementation.

  • SpotlessTask runs the formatter for each source file, and generates the
    formatted file into a designated output directory (if format is required)
    • 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 takes as input the output directory of the SpotlessTask,
    and will fail if any outputs are present
  • 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.

With this change, the Spotless Gradle plugin will leverage more of the
core Gradle functionality for incremental builds, thus avoiding some
of the complication present in the previous implementation.

- SpotlessTask runs the formatter for each source file, and generates the
  formatted file into a designated output directory (if format is required)
   - 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 takes as input the output directory of the SpotlessTask,
  and will fail if any outputs are present
- 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.

At this stage, SpotlessCheck does not produce informative error messages
on failure.
For each file output by the format task, the check task generates
a failure message.

This simle implementation is not particularly efficient as it involves
re-running the formatter against the original source file rather than
comparing the source with the formatted output. However, in the regular
case where 'spotlessCheck' will pass, this inefficiency will have no impact.
In order to maintain existing behaviour for 'spotlessCheck'
and 'spotlessApply', the UP-TO-DATE state of these tasks will
be based on the state of the matching SpotlessTask.

While this is a bit of a hack, it gets the tests passing and
this behaviour can be revisited at a later date.
This test demonstrates that fewer format operations are required to
retain the existing incremental behaviour.
@nedtwigg
Copy link
Member

Amazing!! Thanks so much. I gave it a careful readthrough, and the only thing left before merging is the changelog and fixing up createIndependentTask. This PR removes SpotlessTask.setApply()/setCheck() which were public methods. So far as I know, the only place that uses createIndependentTask uses it like this, which doesn't make any sense with setApply()/setCheck().

https://github.com/diffplug/blowdryer-diffplug/blob/a9c34895a00c4a7ee2e76db3545aa6a12bd4effa/src/main/resources/spotless/freshmark.gradle#L47-L48

All of these benefits are definitely worth this small breaking change. I also think createIndependentTask was probably a mistake, and could be better implemented by #558 (comment).

I'll do some integration testing with this tomorrow and take a stab at the changelog.

@nedtwigg nedtwigg merged commit 289f916 into diffplug:master May 14, 2020
@jbduncan
Copy link
Member

Woo! Happy to see this merged in. Thank you very much @bigdaz. 👍

@nedtwigg
Copy link
Member

Released in plugin-gradle 4.0.0

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.

3 participants