Add goimports functionality to DocumentFormatting #298
Conversation
README.md
Outdated
| /** | ||
| * goimportsLocalPrefix sets the local prefix (comma-separated string) that goimports will use. | ||
| * | ||
| * Defaults to emptry string if not specified. |
langserver/config.go
Outdated
|
|
||
| // GoimportsLocalPrefix sets the local prefix (comma-separated string) that goimports will use | ||
| // | ||
| // Defaults to emptry string if not specified. |
langserver/format.go
Outdated
|
|
||
| if h.config.GoimportsEnabled { | ||
| imports.LocalPrefix = h.config.GoimportsLocalPrefix | ||
| formatted, err = imports.Process("", formatted, nil) |
There was a problem hiding this comment.
So the file is actually formatted twice using cfg.Print (aka gofmt) and imports.Process (aka goimports). Seems unnecessary in my opinion...
In my opinion the code could look something like this:
if h.config.GoimportsEnabled {
// set LocalPrefix
// imports.Process(...)
} else {
// ast.SortImports(...)
// cfg.FPrint(...)
}There was a problem hiding this comment.
Yeah, I thought about this too. I have changed it now. It wasn't completely obvious to me how to do it though, but I ended up changing the imports.Process() call slightly, it seems to work.
|
Windows CI is not happy with this change. Note master branch is passing. |
langserver/format.go
Outdated
| if h.config.GoimportsEnabled { | ||
| imports.LocalPrefix = h.config.GoimportsLocalPrefix | ||
| var err error | ||
| formatted, err = imports.Process(filename, nil, nil) |
There was a problem hiding this comment.
The go-langserver maintains its own file cache, like all other LSP servers do.
So by passing nil as second parameter to Process forces the imports code to actually read the file from disk without trying to read from the internal file cache!
Just move the unformatted variable before this if and pass the unformatted as second argument to Process
|
Unfortunately I dont know what could cause the Windows CI issue and I dont have access to any windows computers. |
langserver/format.go
Outdated
| if h.config.GoimportsEnabled { | ||
| imports.LocalPrefix = h.config.GoimportsLocalPrefix | ||
| var err error | ||
| formatted, err = imports.Process("", unformatted, nil) |
There was a problem hiding this comment.
pass filename instead of "" for the first argument. This should fix the builds.
Original code by @JeanMertz, some tweaks by me. Fixes #272
|
I just squashed the commits now. Thanks for the help and review. |
README.md
Outdated
| * | ||
| * Defaults to true if not specified. | ||
| */ | ||
| goimportsEnabled?: boolean; |
There was a problem hiding this comment.
there is also goreturns. I think the more appropriate config would be an enum like what is done in vscode-go https://github.com/Microsoft/vscode-go/blob/0.6.84/package.json#L563-L574
There was a problem hiding this comment.
Sure: formatTool?: string; ?
that defaults to "goimports" but can also be "gofmt"?
And something like this in config.go?
type FormatTool string
const (
FormatToolGoimports FormatTool = "goimports"
FormatToolGofmt FormatTool = "gofmt"
)There was a problem hiding this comment.
Yup that looks good to me. Thanks!
| @@ -13,6 +13,7 @@ import ( | |||
|
|
|||
| "github.com/pmezard/go-difflib/difflib" | |||
| "golang.org/x/tools/go/buildutil" | |||
| "golang.org/x/tools/imports" | |||
There was a problem hiding this comment.
i don't think this is in our vendor tree. Can you run dep ensure?
There was a problem hiding this comment.
README.md
Outdated
| */ | ||
| goimportsEnabled?: boolean; | ||
| formatTool?: string; |
There was a problem hiding this comment.
given this is typescript, you could write this using a union of string literal types. formatTool?: "goimports" | "gofmt";. I'll update once merged.
|
Thanks so much! |
|
Thank you both for getting this in |
Original code by @JeanMertz, some tweaks by me.
Fixes #272