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: separate imports from formatting #30843

Closed
stamblerre opened this issue Mar 14, 2019 · 6 comments
Closed

x/tools/gopls: separate imports from formatting #30843

stamblerre opened this issue Mar 14, 2019 · 6 comments
Assignees
Labels
Milestone

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Mar 14, 2019

LSP has a separation between formatting a document and organizing imports (the code action "source.organizeImports"). We should follow this by only returning diffs of the import block when we return results from goimports.

/cc @ramya-rao-a

@stamblerre stamblerre added the gopls label Mar 14, 2019
@stamblerre stamblerre self-assigned this Mar 14, 2019
@gopherbot gopherbot added this to the Unreleased milestone Mar 14, 2019
@jbizzle

This comment has been minimized.

Copy link

@jbizzle jbizzle commented Apr 18, 2019

I was actually a little surprised to find this, since the wiki as of today mentions, in the context of configuration with LanguageClient:

" Run gofmt and goimports on save
autocmd BufWritePre *.go :call LanguageClient#textDocument_formatting_sync()

I understood that to mean that results from textDocument/formatting would also include fixing imports.

Am I mis-reading this issue or the wiki configuration example (or both)? If so, would you be able to give some more direction about the expected way to get the imports to be fixed via calls from an lsp client?

FWIW, I did trace the gopls source a bit, and IIUC, the import organizing code would only be invoked as source.Imports via organizeImports via Server.codeAction, and I don't believe there is any connection that I can see between codeAction() and the handler for textFormatting. Not saying this is news to you, just to give you some context on what I do (or do not) understand about the code.

@ianthehat

This comment has been minimized.

Copy link

@ianthehat ianthehat commented Apr 18, 2019

The wiki is a little confusing, it is saying that gopls does the same work that gofmt and goimports do, so there is no need to run them any more. It will do the work of those two tools when you save the document with the settings we suggest, but it does it through two very different code paths (the formatting call and a codeAction)
This bug is about the fact that the implementation of the organizeImports code action is the same code that goimports normally invokes, which is intended to both fix the imports and then format the whole file (because goimports replaced gofmt as the file save hook the way editors used to work). We want to extract the parts of the library that fix the imports and be able to do that on it's own.
The formatting call really does just do the formatting, the same way that gofmt would.

@jbizzle

This comment has been minimized.

Copy link

@jbizzle jbizzle commented Apr 18, 2019

Thanks for the quick reply and clarification. It's possible that there is still a problem, but I'll file it as a separate issue.

@stamblerre stamblerre changed the title x/tools/internal/lsp: separate imports from formatting x/tools/gopls: separate imports from formatting Jul 2, 2019
@suzmue suzmue removed their assignment Aug 28, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 31, 2019

Change https://golang.org/cl/204557 mentions this issue: internal/lsp: do not format the file on import organization

@stamblerre

This comment has been minimized.

Copy link
Contributor Author

@stamblerre stamblerre commented Nov 1, 2019

The above change stops formatting for a file that has no import modifications, but a file will still be formatted if we are adding or deleting imports.

gopherbot pushed a commit to golang/tools that referenced this issue Nov 4, 2019
Updates golang/go#30843
Updates golang/go#35114

Change-Id: Id3f66d20b1ada9e53298b2370214b23b87bb0680
Reviewed-on: https://go-review.googlesource.com/c/tools/+/204557
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
@stamblerre stamblerre assigned heschik and unassigned stamblerre Nov 6, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 7, 2019

Change https://golang.org/cl/205678 mentions this issue: internal/lsp/source: don't format the whole file when adding imports

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.