Format via stdin instead of document file path

[wader]

Fixes double save issues as now we don't have to save
the document to do formatting.

Also fixes save on "Format Document" (now no save is done).

Related to #1037

[wader]

Seems to work fine in my limited tests. Feedback very welcome!

One possible issue with this change is that onWillSaveTextDocument is now used which seems to have a time limit on how long the waitUntil will wait for the edit promise.

An event that is emitted when a text document will be saved to disk.

Note 1: Subscribers can delay saving by registering asynchronous work. For the sake of data integrity the editor might save without firing this event. For instance when shutting down with dirty files.

Note 2: Subscribers are called sequentially and they can delay saving by registering asynchronous work. Protection against misbehaving listeners is implemented as such:

• there is an overall time budget that all listeners share and if that is exhausted no further listener is called
• listeners that take a long time or produce errors frequently will not be called anymore

The current thresholds are 1.5 seconds as overall time budget and a listener can misbehave 3 times before being ignored.

[wader]

I must say that i'm very impressed by the the visual code extension development experience and vscode-go code quality, was very easy to get going!

[wader]

I noticed one thing while reading the code. In isDiffToolAvailable it checks if the diff binary is in PATH but while reading the jsdiff code can't see that it is actually used. Maybe i'm missing something?

[wader]

Maybe with this change {cwd: vscode.workspace.rootPath} is needed?

[ramya-rao-a]

@wader

Thanks for the PR. Am back from the vacation and had some time to dig around my previous work with both using onWillSaveTextDocument and stdin

On the usage of onWillSaveTextDocument:

Telemetry shows that about 1/5th of the Go extension users have had formatting times > 1.5 secs.
If we make this switch, then formatting would stop working for such users. This was the reason that I hadn't made the change to use onWillSaveTextDocument myself when this API was released.

Either, the users have to live with that or we need to be able to provide a fallback

I'll look into what options we have.

On the usage of stdin:

The last time I did this, there were 2 bugs that surfaced due to which I had to revert the change

• Format removes imports of vendored packages in use #613 Format removes imports of vendored packages in use
• goimports fails to consider global variables in package #630 goreturns fails to consider global variables in package

I believe these will occur again with your changes as well?

[ramya-rao-a]

I noticed one thing while reading the code. In isDiffToolAvailable it checks if the diff binary is in PATH but while reading the jsdiff code can't see that it is actually used.

We use isDiffToolAvailable to decide whether or not to send the -d flag to the formatting tool.

[ramya-rao-a]

Also once you have the stdin approach working, we don't really need to use the onWillSaveTextDocument directly. We can deprecate go.formatOnSave and rely on editor.formatOnSave (which internally uses the onWillSaveTextDocument event) instead.

[wader]

@ramya-rao-a Thanks for the feedback

About diff i just found that gofmt actually uses external diff https://github.com/golang/go/blob/master/src/cmd/gofmt/gofmt.go#L257 i just assumed the diff was done in go somehow 😄

Hopefully I will have some time to look into this again this week. Yeah i also suspect that my change will reintroduce those issues again, will try to to see if it can be fixed.

[ramya-rao-a]

Closing this in favor of e95eed6