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/cmd/goimports: import grouping depends on which imports were already there #19190

Closed
cespare opened this issue Feb 19, 2017 · 5 comments
Closed

Comments

@cespare
Copy link
Contributor

@cespare cespare commented Feb 19, 2017

Please answer these questions before submitting your issue. Thanks!

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

Go 1.8

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

linux/amd64

What did you do?

This is on the latest version of goimports (golang/tools@8e779ee).

Consider this test program:

package main

import (
	"time"
)

func main() {
	_ = snappy.Encode
	_ = unix.Mmap
	_ = time.Parse
}

It's missing two imports. If we run goimports -w fmt.go, we'll get

package main

import (
	"time"

	"github.com/golang/snappy"
	"golang.org/x/sys/unix"
)

func main() {
	_ = snappy.Encode
	_ = unix.Mmap
	_ = time.Parse
}

This is what I expect. However, if we delete the unix import:

package main

import (
	"time"

	"github.com/golang/snappy"
)

func main() {
	_ = snappy.Encode
	_ = unix.Mmap
	_ = time.Parse
}

and then run goimports again, we get this grouping:

package main

import (
	"time"

	"golang.org/x/sys/unix"

	"github.com/golang/snappy"
)

func main() {
	_ = snappy.Encode
	_ = unix.Mmap
	_ = time.Parse
}

In this second case, I'd expect the file to be the same as the first case instead of introducing a new grouping.

/cc @bradfitz

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Feb 19, 2017

I don't have time to work on this myself.

@bradfitz bradfitz modified the milestones: unw, Unreleased Feb 19, 2017
@cespare
Copy link
Contributor Author

@cespare cespare commented Feb 19, 2017

/cc @haya14busa, who did some recent goimports work, in case they'd like to look into it.

@haya14busa
Copy link
Contributor

@haya14busa haya14busa commented Feb 20, 2017

Thank you for cc. I think I can work on it.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 1, 2017

CL https://golang.org/cl/37552 mentions this issue.

@haya14busa
Copy link
Contributor

@haya14busa haya14busa commented Mar 1, 2017

I explained what was the problem and how I fixed it in cl/37552, but the fix is a bit heuristic... It works better for most cases though.

cl/37552 works good as far as I know and the change is small, but I welcome better solution if you find more elegant solution.

@golang golang locked and limited conversation to collaborators Aug 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.