Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Add goimports functionality to the DocumentFormatting #272

Closed
JeanMertz opened this issue May 7, 2018 · 5 comments
Closed

Add goimports functionality to the DocumentFormatting #272

JeanMertz opened this issue May 7, 2018 · 5 comments

Comments

@JeanMertz
Copy link

Not sure if this is something you'd consider, but it would be nice if it was possible to auto add missing imports.

I use this diff locally, which seems to do the trick, but I haven't tested this thoroughly yet:

diff --git a/langserver/format.go b/langserver/format.go
index 7678fc9..6ab56bd 100644
--- a/langserver/format.go
+++ b/langserver/format.go
@@ -4,13 +4,13 @@ import (
        "bytes"
        "context"
        "fmt"
-       "go/ast"
        "go/parser"
        "go/printer"
        "go/token"
        "path"
 
        "golang.org/x/tools/go/buildutil"
+       "golang.org/x/tools/imports"
 
        "github.com/sourcegraph/go-langserver/langserver/util"
        "github.com/sourcegraph/go-langserver/pkg/lsp"
@@ -33,8 +33,6 @@ func (h *LangHandler) handleTextDocumentFormatting(ctx context.Context, conn jso
                return nil, err
        }
 
-       ast.SortImports(fset, file)
-
        var buf bytes.Buffer
        cfg := printer.Config{Mode: printer.UseSpaces | printer.TabIndent, Tabwidth: 8}
        err = cfg.Fprint(&buf, fset, file)
@@ -43,6 +41,12 @@ func (h *LangHandler) handleTextDocumentFormatting(ctx context.Context, conn jso
        }
 
        b := buf.Bytes()
+
+       b, err = imports.Process("", b, nil)
+       if err != nil {
+               return nil, err
+       }
+
        orig, err := h.readFile(ctx, params.TextDocument.URI)
        if err != nil {
                return nil, err
@keegancsmith
Copy link
Member

I'm happy with changing our default to using goimports. If someone prefers plain old go fmt then we can provide an option later. Can you send a PR?

@lloiser
Copy link
Contributor

lloiser commented May 8, 2018

#83 is probably also needed to get this working, right?

@JeanMertz
Copy link
Author

Interesting @lloiser. I guess that answers the issue I posted here:

sublimelsp/LSP#319

So yes, the above "works", but as explained in the above issue, it's not very user friendly (but the current gofmt isn't either, since it suffers from the same problem).

@maximbaz
Copy link

I think having goimports functionality is very useful. Since #83 is merged and you have the working patch, @JeanMertz would you please create the PR?

@otto-md
Copy link
Contributor

otto-md commented Jul 22, 2018

It would be very nice if it was possible to also configure the LocalPrefix if this is implemented.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants