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: -local adds local packages before external packages #31646

Open
rayjcwu opened this issue Apr 24, 2019 · 6 comments

Comments

@rayjcwu
Copy link

commented Apr 24, 2019

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

$ go version
go version go1.11.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/raywu/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/raywu/go:/Users/raywu"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.11.2/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.11.2/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/v8/q5brnv35699835dr_mt01t0m0000gn/T/go-build432413042=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Use goimports to add missing timeutil import, which is our local package.

package main

import (
        "fmt"

        "github.com/cespare/wait"
)

func main() {
        fmt.Println("asdf")
        var wg wait.Group
        wg.Wait()
        timeutil.MustParseRFC3339("2019-01-01T00:00:00Z")
}

What did you expect to see?

▶ cat abc.go | ~/goimports -local liftoff/
package main

import (
	"fmt"

	"github.com/cespare/wait"

	"liftoff/go/timeutil"
)

func main() {
	fmt.Println("asdf")
	var wg wait.Group
	wg.Wait()
	timeutil.MustParseRFC3339("2019-01-01T00:00:00Z")
}

What did you see instead?

▶ cat abc.go | ~/goimports -local liftoff/
package main

import (
	"fmt"

	"liftoff/go/timeutil"

	"github.com/cespare/wait"
)

func main() {
	fmt.Println("asdf")
	var wg wait.Group
	wg.Wait()
	timeutil.MustParseRFC3339("2019-01-01T00:00:00Z")
}

@gopherbot gopherbot added this to the Unreleased milestone Apr 24, 2019

@agnivade agnivade changed the title x/tools/cmd/goimports: use -local but add local packages before external packages x/tools/cmd/goimports: -local adds local packages before external packages Apr 24, 2019

@agnivade

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

This seems arbitrary either way, no? It chose something, so that should be fine, just like the rest of the spirit of gofmt?

@rayjcwu

This comment has been minimized.

Copy link
Author

commented Apr 24, 2019

Help message said "put imports beginning with this string after 3rd-party packages; comma-separated list" though. Also if the import block is

import (
        "fmt"
        "github.com/cespare/wait"
)

, then goimports will change it to

import (
	"fmt"

	"github.com/cespare/wait"

	"liftoff/go/timeutil"
)
@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Okay, if it contradicts the docs, that might be reason enough to change it.

I don't object if you send a CL. Others might argue it's better to fix the docs, but I'll let them make that case if so.

@af-engineering

This comment has been minimized.

Copy link

commented May 1, 2019

We're seeing this as well. Our CI catches badly formatted imports and they've been failing recently, even though everyone has setup filewatchers in Goland with the -local option. goimports used to do what the docs mentioned (i.e., putting local packages last), so this is a regression.

Specifically, goimports does not reorganize the below:

import (
    "net/http"
    "strings"

    "github.com/local/repo/pkg/test"

    "github.com/pkg/errors"

    "github.com/local/repo/bar"
    "github.com/local/repo/foo"
)
@rayjcwu

This comment has been minimized.

Copy link
Author

commented May 2, 2019

@af-engineering for Goland you can fix that by using goland settings https://youtrack.jetbrains.com/issue/GO-7321

@gopherbot gopherbot added the Tools label Sep 12, 2019

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