Spotless plugin-gradle "modern"

[nedtwigg]

Currently, the plugin id for Spotless is com.diffplug.gradle.spotless and it is backward-compatible all the way to Gradle 2.x. Soon, it will cohabitate with com.diffplug.spotless, which will require Gradle 5.4+, drops all deprecated code, and adopts all latest-available Gradle APIs. If your build completes without warnings on the old id, then all you'll need to do is swap the new id with no further changes.

It will probably be some weeks/months before com.diffplug.spotless is ready. In the meantime, if you specify -PspotlessModern=true, then you'll be using the new plugin already. We are currently running our test suite using this flag, to ensure that the transition is seamless. (PR #598)

The whole point of Spotless is that formatting doesn't matter, so it follows that code formatting tools really don't matter. This is a unique opportunity to drop confusing features, but anything that gets dropped needs to have a very clear migration path, or else we'll just keep supporting it at the new id, likely forever.

[LifeIsStrange]

Here's the list of autostyle changes that could qualify as breaking change enhancements:
https://github.com/autostyle/autostyle/blob/master/CHANGES.md
For example using a build cache.
Incremental build would be nice too

[bigdaz]

@nedtwigg The more I work with this approach, the less I feel like it's going to work. In order to really move to use the latest Gradle APIs, we'll want to switch the tasks and extensions to use Provider for input/output properties. This way we can remove the afterEvaluate call that wires extension values into the tasks.

Instead of trying to do this in a single branch, what do you think of using 2 different Git branches and publishing the 2 plugins separately? There will be a bit more overhead in keeping the branches in sync, and we'd need a second CI pipeline, but I think we can avoid making nasty compromises in the modern implementation.

WDYT?

[nedtwigg]

@LifeIsStrange, as noted in #616, we already support incremental build and the build cache.

@bigdaz I definitely don't want nasty compromises! How about if SpotlessTaskModern and SpotlessPluginModern etc. don't extend the base classes at all? That should provide just as much flexibility as if they were in a new branch. Also, feel free to just @Ignore any troublesome tests.

I am absolutely killing the old implementation, but I would like to have them parallel in the same source tree. Before releasing, I will do the following transformation:

• SpotlessTaskModern -> SpotlessTask
• SpotlessTask -> SpotlessTaskLegacy

But it's easier for me to understand and maintain your changes if I can review and test them parallel to the legacy implementation. I don't mean to burden you with the testing part of it though, feel free to dump that part on me.

[bigdaz]

Thanks Ned. I understand your motivation, and I'll keep working to do the minimal amount of work so that we can avoid creating tasks eagerly. Yes, and this might involve removing the shared base class for certain types.

At a later date you might want to change FormatExtension, SpotlessExtension and the various tasks to use Provider<X> for property types: this will allow us to get rid of the project.afterEvaluate that's required to ensure the extension is completely populated before reading, and will also permit more idiomatic wiring of task inputs/outputs. Right now, we really don't want to fork FormatExtension for SpotlessModern, since there are many subclasses.

[nedtwigg]

Feel free to break whatever you would like, including FormatExtension, and I can put them back together / abandon the legacy as needed. My vague sense was that these:

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtensionBase.java

Lines 203 to 210 in 183c0b4

	protected <T extends FormatExtension> void configure(String name, Class<T> clazz, Action<T> configure) {
	T value = maybeCreate(name, clazz);
	configure.execute(value);
	}
	@SuppressWarnings("unchecked")
	private <T extends FormatExtension> T maybeCreate(String name, Class<T> clazz) {
	FormatExtension existing = formats.get(name);

could become List<Action<T>> in the modern side, which get evaluated one-by-one on task configuration, and that would allow us to remove the afterEvaluate without making changes to FormatExtension. I get the value of TaskProvider<>, but I haven't yet gotten my head around the value of Provider<> for inputs and outputs. But given how it's a big feature, I think it's a limitation of my understanding, so I'm open to whatever changes you think are best, back-compat be damned.

[nedtwigg]

Released! Thanks for all the help @bigdaz.