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

Pass file to format to custom formatter #1603

Open
mrmeku opened this issue Jul 2, 2021 · 6 comments
Open

Pass file to format to custom formatter #1603

mrmeku opened this issue Jul 2, 2021 · 6 comments

Comments

@mrmeku
Copy link

mrmeku commented Jul 2, 2021

Describe the bug

Currently the vscode-go extension has code paths to support running a custom go formatting tool. However, those code paths stop short of actually passing the file that should be formatted as an argument to the formatter. This makes it so that a custom formatting tool cannot actually be run since it has no way of knowing what file it should format

Steps to reproduce the behavior:

  1. Set "go.formatTool" to a path to a custom tool
  2. Attempt to format a go file
  3. See that that the custom tool is not passed an argument of a file to format
@gopherbot gopherbot added this to the Untriaged milestone Jul 2, 2021
mrmeku pushed a commit to mrmeku/vscode-go that referenced this issue Jul 2, 2021
mrmeku pushed a commit to mrmeku/vscode-go that referenced this issue Jul 2, 2021
@gopherbot
Copy link
Collaborator

Change https://golang.org/cl/332529 mentions this issue: src/goFormat.ts: pass filename to custom formatters

@hyangah
Copy link
Contributor

hyangah commented Jul 2, 2021

It's tricky to assume what a "custom" formatter takes as args. Is it not possible to utilize "go.formatFlags"? Maybe implement variables replacement similar to vscode's launch.json/task.json variables if necessary?

The extension runs the format tool inside the directory of the document (assuming go fmt style behavior that takes the package path or the package in the current directory)

@mrmeku
Copy link
Author

mrmeku commented Jul 2, 2021

My intention is that you would wrap the custom formatter in a script so that you can transform why the extension passes into the form the formatter needs.

Also happy to introduce a template variable if that's preferred

@findleyr findleyr added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. FeatureRequest labels Jul 7, 2021
@findleyr findleyr modified the milestones: Untriaged, Backlog Jul 7, 2021
@hyangah
Copy link
Contributor

hyangah commented Jul 12, 2021

@mrmeku We discussed this in our internal team meeting and we hope to stick with the go fmt style that takes the optional 'packages' argument, in order to avoid breaking other existing use cases. Can you please change to support variable substitutions for go.formatFlags? Thanks!

@hyangah hyangah removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 13, 2021
@gopherbot
Copy link
Collaborator

Change https://go.dev/cl/446298 mentions this issue: docs/settings: make the custom formatter support more visible

@gopherbot
Copy link
Collaborator

Change https://go.dev/cl/446296 mentions this issue: test/gopls: narrow the scope of fake formatTool setting

gopherbot pushed a commit that referenced this issue Nov 10, 2022
The extension supports custom formatter. When gopls is used, this
is done by intercepting the formatting request from the middleware
and passing it to the legacy format provider that calls the specified
tool. The "Nonexistent formatter" test is meant to take this
middleware code path and let the legacy format provider fail by
setting `go.formatTool` with a bogus, non-existing tool name.

This setting doesn't need to affect other tests though.
Limit this setting to only the nonexistent formatter, by starting
a new language server instance for each test.

Additionally this fixes bugs:
- wait to show the document to format - the legacy formatter doesn't
  run if there is no visible document. (I don't know why we don't format
  non-visible document but that's how it's written goFormat.ts:24).
- close the open editor after each test.

For #1603

Change-Id: I3486b299aa2a2660fb971fed54d3f941be13698d
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/446296
TryBot-Result: kokoro <noreply+kokoro@google.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
gopherbot pushed a commit that referenced this issue Nov 10, 2022
Custom formatter support was added in #1238.
But due to the limitation in the VS Code setting UI (which assumes
the acceptable values list is static) and the setting validation
logic (VS Code thinks it's an error to use a value outside of the
supplied enum list is invalid), this is not visible to users.

Instead, this change introduces an extra enum 'custom' for the
"go.formatTool" setting. If this is chosen, the extension uses
the tool specified as `customFormatter` in the "go.alternateTools"
setting section for formatting.

The extension expects the custom formatter to accept input as STDIN
and output the result as STDOUT. Users can also supply "go.formatFlags".

Changed the descriptions to use markdown - which allows to reference
other settings ("`#...#`"). The document generation tool does not
handle this special syntax nicely but I hope this isn't too confusing.

I wish we could add an extra validation on the allowed value for
"go.formatTool" in favor of this new 'custom' option. I am not adding
the validation in this CL because there could be users who depend
on this behavior.

For #1238
For #1603
For #2503

Change-Id: I5d9564f331845b6b07f0b54148834118404f3553
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/446298
Reviewed-by: Jamal Carvalho <jamal@golang.org>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants