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

x/tools/gopls: allow enabling gofumpt extra rules #56403

Open
maxbrunet opened this issue Oct 24, 2022 · 5 comments
Open

x/tools/gopls: allow enabling gofumpt extra rules #56403

maxbrunet opened this issue Oct 24, 2022 · 5 comments
Labels
FeatureRequest gopls Issues related to the Go language server, gopls. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.

Comments

@maxbrunet
Copy link

maxbrunet commented Oct 24, 2022

gopls version

master (8166dca1c) (latest release: v0.9.5)

go env

N/A

What did you do?

Currently gofumpt can be enabled: https://go.googlesource.com/tools/+/refs/heads/master/gopls/doc/settings.md#gofumpt-bool

But extra rules cannot be used: mvdan/gofumpt#extra-rules-behind--extra

What did you expect to see?

An option to enable extra rules (e.g. formatting.gofumptExtraRules bool). Suggested implementation: golang/tools#410

What did you see instead?

No option to enable extra rules.

Editor and settings

"gopls": { "formatting.gofumpt": true }

Logs

N/A

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Oct 24, 2022
@gopherbot gopherbot added this to the Unreleased milestone Oct 24, 2022
@maxbrunet maxbrunet changed the title x/tools/gopls: x/tools/gopls: allow enabling gofumpt extra rules Oct 24, 2022
@gopherbot
Copy link

gopherbot commented Oct 24, 2022

Change https://go.dev/cl/444656 mentions this issue: gopls, internal/lsp: allow enabling gofumpt extra rules

@dle8 dle8 added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Oct 26, 2022
@findleyr
Copy link
Contributor

findleyr commented Oct 27, 2022

We generally try to avoid new configuration when possible, but this may be important as we already integrate with gofumpt.

Curious what @mvdan thinks of this request.

@mvdan
Copy link
Member

mvdan commented Oct 27, 2022

That flag only turns on one additional rule, and it's a somewhat controversial one. Looking ahead, that kind of optional cleanup should rather be a "quickfix" step that the user opts into, rather than formatting that happens automatically. So I don't oppose adding the feature in the future (e.g. as a quickfix), but I do lean against adding "extra rules" as a formatting option. At least for now, given the only rule that it turns on.

@findleyr
Copy link
Contributor

findleyr commented Oct 28, 2022

@mvdan that sounds like a great idea: if gofumpt were to provide an API for these types of additional formatting rules, we could definitely offer them as a quick-fix, via an analyzer that could be enabled/disabled independently of gofumpt formatting.

@maxbrunet what do you think about this idea?

@maxbrunet
Copy link
Author

maxbrunet commented Oct 31, 2022

Thank you for the replies, it sounds reasonable. I am not familiar enough with LSP and the requirements for a code action, but I have opened mvdan/gofumpt#251 to start the discussion about what it would look like there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest gopls Issues related to the Go language server, gopls. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants