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: group imports together for the local module #32049

Open
gracenoah opened this issue May 15, 2019 · 23 comments

Comments

@gracenoah
Copy link

@gracenoah gracenoah commented May 15, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12 linux/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

not relevant

What did you do?

Saved a file in vs code (configured to use gopls), letting gopls fix imports.

What did you expect to see?

Imports grouped into 3 sections: standard library imports, third party library imports, local module imports

What did you see instead?

Only two sections: standard library imports, all other imports


Maybe this is a bike-sheddy style question, but I would like to be able to use gopls but also stick to the old goimports -local x behaviour of splitting local dependencies out.

If it's too controversial, I'd be happy with a flag, but maybe it's simple enough to make this behaviour default, especially now that go modules make it clear what's "local".

@gopherbot gopherbot added this to the Unreleased milestone May 15, 2019
@gopherbot gopherbot added the gopls label May 15, 2019
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented May 15, 2019

I think this is ultimately something that we would put behind a config (even if it is on by default). We intend to add different configurations to formatting and imports, but first we will need to fix #30843.

@stamblerre stamblerre changed the title x/tools/cmd/gopls: group imports together for the local module x/tools/gopls: group imports together for the local module Jul 2, 2019
@stamblerre stamblerre assigned matloob and unassigned suzmue Aug 8, 2019
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Aug 8, 2019

@matloob: This is something that we could implement through a suggested fix through go/analysis.

@stamblerre stamblerre removed the Suggested label Aug 8, 2019
@cespare

This comment has been minimized.

Copy link
Contributor

@cespare cespare commented Aug 13, 2019

@stamblerre from a cursory glance through the code, it seems like it would be straightforward to plumb through the imports.Options.Env.LocalPrefix config today, whereas #30843 seems like a substantial change.

Would you accept a PR which adds a temporary config option for now? In the future it would be replaced by some other mechanism once #30843 is in.

I ask because the -local option is how we organize all imports at our company, and not having a way to get the same result makes gopls hard to adopt.

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Aug 13, 2019

That sounds like a reasonable temporary measure. I'll leave the final decision to @suzmue though, since she has much more expertise with goimports.

@suzmue

This comment has been minimized.

Copy link
Contributor

@suzmue suzmue commented Aug 13, 2019

That sounds good to me as well.

@smyrman

This comment has been minimized.

Copy link

@smyrman smyrman commented Sep 10, 2019

Is it relevant to utilize the path in go module to determine the local path?

@sushicw

This comment has been minimized.

Copy link

@sushicw sushicw commented Sep 23, 2019

I'm running into this pain point as well.

@cespare Are you still working on a PR to add a config flag for this?

@cespare

This comment has been minimized.

Copy link
Contributor

@cespare cespare commented Sep 23, 2019

@sushicw Not actively. It's on my to-do list. If you'd like to do it, go ahead.

I expect I'll get to this in the next 1-2 months.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 1, 2019

Change https://golang.org/cl/204820 mentions this issue: internal/lsp: add LocalPrefix configuration

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Nov 1, 2019

The change above adds a LocalPrefix configuration, but I do think it would be worth considering a default value that uses the name of the main module. Let's start with this configuration though, and then consider the further improvements.

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

Change-Id: I64e5201170b5be8b470c436264e18e12ec8d12f2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/204820
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
@zikaeroh

This comment has been minimized.

Copy link

@zikaeroh zikaeroh commented Nov 5, 2019

I went to try this out, but couldn't get it to work (even added some logging where the ProcessEnv gets created to make sure it was plumbing it down). Turns out that the VS Code Go extension is the one doing the formatting, and not gopls. Is there some trick I've missed to make use of gopls as the formatter rather than an external tool?

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Nov 5, 2019

@zikaeroh: What are your VSCode settings (Ctrl + Shift + P -> Preferences: Open Settings (JSON))? As long as you haven't explicitly disabled formatting in go.experimentalLanguageServerFlags, I don't see why it wouldn't go through gopls.

@zikaeroh

This comment has been minimized.

Copy link

@zikaeroh zikaeroh commented Nov 5, 2019

Ah, that's it. I set the format flag to false, probably because format-on-save wasn't behaving the way I wanted the first time I used gopls a while ago. I'll retest when I get a chance later today.

@alexanderbez

This comment has been minimized.

Copy link

@alexanderbez alexanderbez commented Nov 5, 2019

Can someone share the VSCode settings needed to get this to work, please?

Here is what I currently have:

    "go.useLanguageServer": true,
    "go.languageServerExperimentalFeatures": {
        "documentLink": false,
        "incrementalSync": true,
    },
    "gopls": {
        "usePlaceholders": false,
        "completionDocumentation": true,
    },
    "[go]": {
        "editor.codeActionsOnSave": {
            "source.organizeImports": true,
        },
        "editor.codeActionsOnSaveTimeout": 3000,
        // "editor.defaultFormatter": "ms-vscode.Go",
        "editor.formatOnSave": true,
        "editor.snippetSuggestions": "none",
    },
    "editor.formatOnSave": true

I would like to use the new LocalPrefix config/flag.

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Nov 5, 2019

You will need to add "local": "your module path" to the "gopls" settings block.

@zikaeroh

This comment has been minimized.

Copy link

@zikaeroh zikaeroh commented Nov 5, 2019

Had some time so went to test it out... I remember why I disabled gopls's formatting, it's pretty broken for me. 🙁

I'll have to file a new bug for it, but doing something like moving some imports around and saving reformats my imports very wrong, with some disappearing and sometimes even quotes getting misplaced. That, and the local option doesn't appear to have done it for me. I'll try to do better testing tonight, but I foresee staying with goimports externally as that's been working very well up to this point.

EDIT: I got it to work, but only if I use "organize imports". Personally, I want the old goimports behavior of "save the file and everything just works out", so I hope the gopls scheme isn't separate actions for everything I have to remember to run...

@alexanderbez

This comment has been minimized.

Copy link

@alexanderbez alexanderbez commented Nov 5, 2019

You will need to add "local": "your module path" to the "gopls" settings block.

I get unexpected config local when doing this.

gopls version:

golang.org/x/tools/gopls v0.1.7
    golang.org/x/tools/gopls@v0.1.7 h1:YwKf8t9h69++qCtVmc2q6fVuetFXmmu9LKoPMYLZid4=
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Nov 5, 2019

@zikaeroh: Can you file a new issue with a repro for the case where gopls formatting messes things up? All you should need are the format and code action on save hooks described here: https://github.com/golang/tools/blob/master/gopls/doc/vscode.md#vscode.

@alexanderbez: Sorry, forgot to mention that you will also need to install the latest pre-release of gopls - run this command from a temporary directory: go get golang.org/x/tools/gopls@v0.2.0-pre2.

@zikaeroh

This comment has been minimized.

Copy link

@zikaeroh zikaeroh commented Nov 6, 2019

Opened #35388. I do have all of the recommended options set including the on-save options and an extended timeout, but no dice. goimports works great. :P

@alexanderbez

This comment has been minimized.

Copy link

@alexanderbez alexanderbez commented Nov 6, 2019

got it working, but the importing is definitely not right. Probably what @zikaeroh is alluding to. It's not even grouping stdlib imports.

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Nov 6, 2019

@alexanderbez: Does it work if you remove an import? goimports won't necessarily move things around unless things are changed (see #20818).

@alexanderbez

This comment has been minimized.

Copy link

@alexanderbez alexanderbez commented Nov 6, 2019

goimports works perfectly. But when I simply save (with the new local config) I get:

import (
	"fmt"
	"github.com/cosmos/cosmos-sdk/codec"
	sdk "github.com/cosmos/cosmos-sdk/types"
	abci "github.com/tendermint/tendermint/abci/types"
	"os"
	"sort"
	"strings"
	"syscall"
)

what I expect:

import (
	"fmt"
	"os"
	"sort"
	"strings"
	"syscall"

	"github.com/cosmos/cosmos-sdk/codec"

	sdk "github.com/cosmos/cosmos-sdk/types"
	abci "github.com/tendermint/tendermint/abci/types"
)

assuming "github.com/cosmos/cosmos-sdk" is my local prefix.

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Nov 6, 2019

What is the content of the original file?

@stamblerre stamblerre assigned stamblerre and unassigned matloob Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.