Skip to content

Fix race conditions with new spotless API#10

Merged
badsyntax merged 52 commits intomasterfrom
new-spotless-api
May 11, 2020
Merged

Fix race conditions with new spotless API#10
badsyntax merged 52 commits intomasterfrom
new-spotless-api

Conversation

@badsyntax
Copy link
Copy Markdown
Owner

@badsyntax badsyntax commented May 8, 2020

This PR fixes all the issues with race conditions, as outlined in this gist.

At a high-level, the formatting no longer relies on disk operations. Instead standard streams (stdin, stdout, stderr) are used to pass document text to spotless and get back formatted text. Various changes were made to vscode-gradle (microsoft/vscode-gradle#348) and spotless (diffplug/spotless#568) to support this. This also means we can format dirty files without saving them.

TODO

  • Depends on Fix types package and improve extension API microsoft/vscode-gradle#348 being completed and new vscode-gradle package released
  • New spotless version released (IDE hook diffplug/spotless#568)
  • Integration tests working in CI
  • Add note to readme about not formatting untitled files
  • Add tests for formatting invalid java files
  • Add tests for formatting clean files
  • Cancel task when formatting is cancelled
  • Add tests for massive java files Tests would be too flaky. I've added an example of a massive file and have manually tested the formatting on it.
  • Check performance on massive projects
  • Add acknowledgements section to readme
  • Fix activation events (*.gradle instead of gradlew)
  • Add note to readme about requiring minimum spotless-gradle version
  • Use new spotless -PspotlessIdeHookUseStdin -PspotlessIdeHookUseStdOut params
  • Add more supported languages - add tests for this
  • Update gif to show formatting on dirty files
  • Add architecture diagram

Supersedes #5

badsyntax added 4 commits May 8, 2020 09:46
The new spotless API allow us to use the stdin, stdout & stderr streams without ever writing to disk.

This solves all of the race conditions.
Copy link
Copy Markdown
Owner Author

@badsyntax badsyntax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments

Comment thread src/spotless.ts Outdated
Comment thread test-fixtures/gradle-project/.vscode/settings.json Outdated
Comment thread test-fixtures/gradle-project/.vscode/settings.json Outdated
Comment thread src/spotless.ts Outdated
@badsyntax badsyntax force-pushed the new-spotless-api branch from 472c9dd to d07d1b9 Compare May 8, 2020 20:42
Comment thread src/FixAllProvider.ts Outdated
@badsyntax
Copy link
Copy Markdown
Owner Author

badsyntax commented May 9, 2020

Tests are failing in Windows. I've replicated this behaviour when running the extension in Windows 10. Attempt to debug by using the gradle wrapper, as outlined in this comment: diffplug/spotless#568 (comment)


After some initial testing, it is working as expected using the gradle wrapper:

gradlew.bat spotlessApply -PspotlessIdeHook=C:\Users\Richard\Desktop\vscode-spotless-gradle\test-fixtures\gradle-project\src\main\java\gradle\project\App.java -PspotlessIdeHookUseStdIn -PspotlessIdeHookUseStdOut --quiet < stdin.txt

...with the following contents in stdin.txt:

package gradle.project;public class App {public static void main(String[] args) {System.out.println("app");}}

Using spotless-gradle version 13700cbe45344f45cbef79cbba94aec6724757f8 (latest)


Next steps: use latest spotless-gradle version in vscode, check the windows paths aren't being munged.


I've discovered the issue lies within vscode-gradle and how i'm chunking stdout based on newlines. Not sure the best approach to fixing this but at least it's fixable and not a problem with gradle or spotless-gradle.

@badsyntax
Copy link
Copy Markdown
Owner Author

Created a bug report in vscode-gradle to fix the streaming issues in Windows: microsoft/vscode-gradle#355

badsyntax added 4 commits May 9, 2020 21:45
I've had great difficulty find a nice way to test cancelled formatting, so this will have to do.
badsyntax added 10 commits May 11, 2020 14:18
It's not a known language identifer
It's not a known language type
This extension provides a bunch of language identifiers which we don't want when running tests
For some reason the kotlin formatting timeouts on MacOS in CI
@badsyntax badsyntax marked this pull request as ready for review May 11, 2020 14:54
@badsyntax
Copy link
Copy Markdown
Owner Author

@nedtwigg This PR is pretty much ready to go. It was more work than I expected. I had all types of issues with CI, as usual haha. Integration tests (for Linux, Windows & MacOS) are now passing consistently.

Now I just need diffplug/spotless#568 merged and a new version released before I can release these changes.

I had to make a bunch of a changes to vscode-gradle including breaking public API changes. As VS Code extensions don't support beta/dev releases, this extension (vscode-spotless-gradle) is currently broken for users because of those changes.

@nedtwigg
Copy link
Copy Markdown

Now published in Spotless 3.30.0.

@badsyntax
Copy link
Copy Markdown
Owner Author

LGTM!

@badsyntax badsyntax merged commit 70dd77a into master May 11, 2020
@badsyntax badsyntax deleted the new-spotless-api branch May 11, 2020 18:12
@badsyntax
Copy link
Copy Markdown
Owner Author

Released with 0.1.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.

2 participants