IDE hook

[nedtwigg]

We currently have a flag which allows formatting specific files, -PspotlessFiles, documented here

You can target specific files by setting the spotlessFiles project property to a comma-separated list of file patterns:
cmd> gradlew spotlessApply -PspotlessFiles=my/file/pattern.java,more/generic/.*-pattern.java
The patterns are matched using String#matches(String) against the absolute file path

There are two problems with this:

1. String.matches(String regex) is a bummer of an API for paths, because you have to regex-escape dots and backslashes. We've wasted a lot of user time on this surprising behavior (Maven flag -DspotlessFiles= ineffective #462, Problems with SpecificFilesTest #528, Relative Path Support for spotlessFiles #531, Custom Spotless Task #518).
2. A regex can't declare a file, it can only filter a collection of files specified elsewhere. That means you have to resolve the entire collection even if the regex is only going to match once (see Spotless gradle is super slow and doesn't appear to respect the -PspotlessFiles flag #557). It would be faster to affirmatively to declare "I want to format this file", and then let the tasks decide if they contain that file. From the FileCollection javadoc:

boolean contains​(File file)
Determines whether this collection contains the given file. Generally, this method is more efficient than calling getFiles().contains(file).

This PR won't get merged until after #560 makes it in, too many merge conflicts between them and 560 is more important. But this PR shows how we can easily be much faster with this API. Main goal is to get feedback on the documentation, and alert interested parties to better API on the way.

The stimulus to make me work on this is @badsyntax's VS Code extension. Now our "format one file" performance matters, and it's silly that he has to do a regex-escape just to pass a single file.

[nedtwigg]

Moving a twitter convo here: the VS Code extension (#567) could work better with a stdout API, rather than modifying the file in-place.

Here's the relevant issues: https://gist.github.com/badsyntax/d6b3c0676d9f99a9784327e5004d889d

[badsyntax]

This new API looks great and will make my consuming code a whole lot simpler.

Regarding better VS Code integration, getting spotless to write to stdout would solve the problems mentioned in that gist. Basically VS Code provides various API's (eg CodeAction) to provide code changes for a text document, and saving to disk violates those API's and causes race conditions.
stdout could be exclusive to spotlessApplyOnlyTo, as I don't see a reason to write to stdout when using spotlessApply, but I guess it's up to the spotless authors to decide.

[nedtwigg]

I'm retargeting this PR to be a prototype for an IDE hook. The API currently implemented is this:

./gradlew --quiet -PspotlessIdeHook=C:/Absolute/Path/To/File

• if stderr startsWith IS DIRTY, then the file was dirty, and stdout contains its full formatted contents (in every other case, stdout will be empty)
• if stderr startsWith IS CLEAN, then the file is already clean
• if stderr startsWith DID NOT CONVERGE, then the formatter is misbehaving, and the rest of stderr has useful diagnostic info
• if stderr is empty, then the file is not being formatted by spotless (not included in any target)
• if stderr is anything else, then it's the stacktrace of whatever went wrong

Here are testcases for each of these:

spotless/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/IdeHookTest.java

Lines 79 to 112 in 842bacc

	@Test
	public void dirty() throws IOException {
	runWith("spotlessApply", "--quiet", "-PspotlessIdeHook=" + dirty.getAbsolutePath(), "--stacktrace");
	Assertions.assertThat(output).isEqualTo("abc");
	Assertions.assertThat(error).startsWith("IS DIRTY");
	}
	@Test
	public void clean() throws IOException {
	runWith("spotlessApply", "--quiet", "-PspotlessIdeHook=" + clean.getAbsolutePath());
	Assertions.assertThat(output).isEmpty();
	Assertions.assertThat(error).startsWith("IS CLEAN");
	}
	@Test
	public void diverge() throws IOException {
	runWith("spotlessApply", "--quiet", "-PspotlessIdeHook=" + diverge.getAbsolutePath());
	Assertions.assertThat(output).isEmpty();
	Assertions.assertThat(error).startsWith("DID NOT CONVERGE");
	}
	@Test
	public void outofbounds() throws IOException {
	runWith("spotlessApply", "--quiet", "-PspotlessIdeHook=" + outofbounds.getAbsolutePath());
	Assertions.assertThat(output).isEmpty();
	Assertions.assertThat(error).isEmpty();
	}
	@Test
	public void notAbsolute() throws IOException {
	runWith("spotlessApply", "--quiet", "-PspotlessIdeHook=build.gradle");
	Assertions.assertThat(output).isEmpty();
	Assertions.assertThat(error).contains("Argument passed to spotlessIdeHook must be an absolute path");
	}

@badsyntax you can integration-test against any commit in this PR using jitpack with these instructions (use 'diffplug' for 'USER_OR_ORG')

Let's iterate in this PR until we have an API that makes it easy for your plugin to provide a good user experience.

[badsyntax]

I've had some difficulties consuming the jitpack package.

settlings.gradle

pluginManagement {
  repositories {
    maven {
      url 'https://jitpack.io'
      content {
        includeGroup 'com.github.diffplug.spotless'
      }
    }
    gradlePluginPortal()
  }
  resolutionStrategy {
    eachPlugin {
      if (requested.id.id == 'com.diffplug.gradle.spotless') {
        useModule('com.github.diffplug.spotless:spotless-plugin-gradle:855306c42cd14e9dcbdfd45ab9b7d4693030b608')
      }
    }
  }
}


rootProject.name = 'gradle-project'

Gradle output

FAILURE: Build failed with an exception.

* What went wrong:
A problem occurred configuring root project 'gradle-project'.
> Could not resolve all artifacts for configuration ':classpath'.
   > Could not resolve com.github.diffplug:spotless-lib:1.29.0-SNAPSHOT.
     Required by:
         project : > com.github.diffplug.spotless:spotless-plugin-gradle:855306c42cd14e9dcbdfd45ab9b7d4693030b608
      > Could not resolve com.github.diffplug:spotless-lib:1.29.0-SNAPSHOT.
         > Unable to load Maven meta-data from https://jitpack.io/com/github/diffplug/spotless-lib/1.29.0-SNAPSHOT/maven-metadata.xml.
            > Could not get resource 'https://jitpack.io/com/github/diffplug/spotless-lib/1.29.0-SNAPSHOT/maven-metadata.xml'.
               > Could not GET 'https://jitpack.io/com/github/diffplug/spotless-lib/1.29.0-SNAPSHOT/maven-metadata.xml'. Received status code 401 from server: Unauthorized
   > Could not resolve com.github.diffplug:spotless-lib-extra:1.29.0-SNAPSHOT.
     Required by:
         project : > com.github.diffplug.spotless:spotless-plugin-gradle:855306c42cd14e9dcbdfd45ab9b7d4693030b608
      > Could not resolve com.github.diffplug:spotless-lib-extra:1.29.0-SNAPSHOT.
         > Unable to load Maven meta-data from https://jitpack.io/com/github/diffplug/spotless-lib-extra/1.29.0-SNAPSHOT/maven-metadata.xml.
            > Could not get resource 'https://jitpack.io/com/github/diffplug/spotless-lib-extra/1.29.0-SNAPSHOT/maven-metadata.xml'.
               > Could not GET 'https://jitpack.io/com/github/diffplug/spotless-lib-extra/1.29.0-SNAPSHOT/maven-metadata.xml'. Received status code 401 from server: Unauthorized

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 2s

Seems like it's trying to download packages that require authentication, eg https://jitpack.io/com/github/diffplug/spotless-lib/1.29.0-SNAPSHOT/maven-metadata.xml

I've tried a bunch of things, including checking out the branch locally and publishing to local maven but got a bit confused on how the snapshots work.

[nedtwigg]

@badsyntax Maybe we can chat in realtime on gitter: https://gitter.im/diffplug/spotless

[nedtwigg]

Solution is to add a metadataSources block like this:

  repositories {
    maven {
      url 'https://jitpack.io'
      metadataSources {
        ignoreGradleMetadataRedirection()
        mavenPom()
      }
      content {
        includeGroup 'com.github.diffplug.spotless'
      }
    }

I will update our instructions and also file a bug with JitPack.

[nedtwigg]

Fumble-fingers

[badsyntax]

Cool that change is working for me. Will have a play and give some feedback 👍

[badsyntax]

After integrating this change into vscode I've discovered something else that would be required. Apologies for not realising this beforehand.
To prevent relying on disk operations to prevent race conditions in the editor, I would also need the concept of stdin. From the consuming side, when running the task via the Gradle Tooling API, I could use LongRunningOperation.setStandardInput.
On the producer side (the spotless task), i'm unsure if the Gradle architecture allows you to easily read input streams within the task. I think it depends on the type of task. It'd be great if you could investigate the feasibility of using stdin.
If this is not possible, another potential approach could be to pass in the path of a temporary file via an additional parameter. On the consuming side, i would save the (dirty/unsaved) text file contents to disk outside of the project root (eg /var/folders/some/temp/folder/App.java) and pass that path in. In the spotless task you'd read the contents of that temp file instead of the file specified in spotlessIdeHook.

[nedtwigg]

I see, you'll use stdin to send the possibly-dirty editor state. Sounds like a good plan! Taking a look...

[nedtwigg]

Hmm.. that's too bad there's a race condition when you ask the editor to save. stdin introduces a (small) race condition on the Spotless side:

• Spotless needs the file path of a real existing file so it can ask some questions of .gitattributes and gradle target files, I don't think they play nice with not-existing files (not sure)
• So if VSCode has a deleted file in buffer, where VSCode knows what the path used to be, I'm not sure if Spotless will handle it very well

But we'll see! Taking a quick hack at stdin.

[nedtwigg]

The gradle testkit doesn't have a "set stdin" that I could see, so I didn't do anything to fix the tests. But if the naive "read all bytes on System.in" approach works, then the commit above should work for you.

[badsyntax]

About the race conditions, these are some good points. At the moment the formatting is restricted to files that have already been saved to disk, as I assumed spotless would need to know various things based on the file location in order to format correctly. This is a disclaimer I can add to the README. It'd be great if we could accommodate this scenario, but I see it as a "nice to have" rather than a real issue.

About testing this stdin change, i'm not able to find the package hash in jitpack.

I see the package is in jitpack now. Am testing this change.

[badsyntax]

So this seems to be working correctly, which is great news. I'm able to set the file contents via stdin with the Gradle Tooling API, and am able to get the formatted contents back via stdout.

The working stdin approach is also demonstrated by this command using the gradle wrapper:

./gradlew spotlessApply \
  -PspotlessIdeHook=/Users/richardwillis/Projects/badsyntax/vscode-spotless-gradle/test-fixtures/gradle-project/src/main/java/gradle/project/App.java \
  --quiet \
  <<< 'package gradle.project;public class App {public static void main(String[] args) {System.out.println("app");}}' \
  2> /dev/null

I'm however having some difficulties suppressing the gradle progress logs being written to stdout with tooling API, even with passing the --quiet argument.

--quiet seems to work correctly for other tasks (eg the help task) but not with the spotlessApply task. This behaviour is limited to the tooling api, and works correctly when using the wrapper, as shown above. I'll keep debugging this tomorrow. If you have any ideas though, please let me know.

The gradle testkit doesn't have a "set stdin" that I could see, so I didn't do anything to fix the tests.

I had a look at the testkit API before suggesting stdin and couldn't see any API to allow you to test this, and so was hesitant to suggest this approach. I hope you can find a way to test this! I'll try debug the quiet issue tomorrow and let you know.

[badsyntax]

Ignore my previous comment about issues with the --quiet argument. It's all working nicely, and it's pretty fast too!

You'll see in the gif below that i'm using the input & output streams instead of saving to disk, and it correctly formats a "dirty" state, as well as formatting "on save", and there's no race conditions. Super excited about this!

I'll write some vscode integration tests tomorrow, but the new spotless API is looking good 👍

[nedtwigg]

Great! I'll add some docs and fixup the tests. @bigdaz, do you have an in-progress PR for #560? This PR will cause merge-conflicts for it - I'm happy to take responsibility for fixing them.

[nedtwigg]

@badsyntax Just FYI, you will need to add -PspotlessIdeHookUseStdin -PspotlessIdeHookUseStdOut. Adding those flags makes the code easier to test for me, and perhaps easier to integrate for other IDE plugins. Hopefully doesn't add too much cumbersome for you.

Also, I added a questionable commit 13700cb. When you send a file using the hook, it has to be tested against every single SpotlessTask, and there can be many of them in a large multiproject setup. It would be nice to be able to "return early", so that once the correct task is found, you can do the format and move on, rather than waiting for the file to be tested against every other SpotlessTask as well.

I don't know of a way to "end a build" cleanly from within a task, but I can close System.out and System.err. I'm not sure how your interface works - do you wait for a process to end, or for the streams to close? Anyway, easy to revert that last commit if it's a bad idea, but otherwise I think this is ready to release on our side.

[badsyntax]

@nedtwigg No problem about the changes, I'll update my code.

At the moment, I stream stdout/sterr to the client, but I wait for the gradle process/build to end, using BuildLauncher.run(), before applying the changes collected from stdout. I don't check for stream close. I'll see I can adapt my code to format on stream close, but I worry a bit about the effects of not waiting for the build to finish. I'll need to do some more testing to fully understand the effects.

I've added an early-stage PR (badsyntax/vscode-spotless-gradle#10) if you want to have an initial look at how the vscode extension deals with the streams. (Keep in mind there's still lots I need to do before it's ready for release.)

[bigdaz]

@bigdaz, do you have an in-progress PR for #560? This PR will cause merge-conflicts for it - I'm happy to take responsibility for fixing them.

Yep, this is coming along slowly. I have a rough implementation and all of the tests passing. I expect to add some polish and have something to share next week.

Thanks for the offer, but unless the merge conflicts are severe, I'll likely just absorb them as part of my rebase/cleanup work.

[nedtwigg]

I don't check for stream close. I'll see I can adapt my code to format on stream close, but I worry a bit about the effects of not waiting for the build to finish. I'll need to do some more testing to fully understand the effects.

This is probably premature optimization on my part. If it's trivial then take a look, if it's not then I'll just revert that last commit. There are other ways to speed it up if we need to in the future.

[badsyntax]

@nedtwigg it seems that closing the system streams has no effect on the custom OutputStreams that I set with setStandardOutput and setStandardError

On the consuming side I need to explicitly close the custom OutputStream's after the build has run.
In the consuming code the OutputSteam's content is always streamed back to the client (the vscode extension) before the "spotless format" operation is complete, and the extension code accommodates this. This means if we were to figure out how to close the custom streams early from within the spotless task, it would just work in the extension.

So I guess it's fine to leave that code as is, as it might be providing performance improvements for those running this task without setting custom output streams (eg via the gradle wrapper).

[badsyntax]

Btw badsyntax/vscode-spotless-gradle#10 is now ready, but depends on these changes being released.

[nedtwigg]

Now published in 3.30.0.

[nedtwigg]

Announcement tweet

[jbduncan]

Hey @nedtwigg, sorry for the delay in responding, it's been a busy week for me!

I've just given this a cursory glance, but from what I've seen it LGTM. 👍