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: unnecessary rename of imports #29556

Closed
rsc opened this issue Jan 4, 2019 · 5 comments
Closed

x/tools/cmd/goimports: unnecessary rename of imports #29556

rsc opened this issue Jan 4, 2019 · 5 comments
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Jan 4, 2019

In GOPATH mode:

$ go get gopkg.in/russross/blackfriday.v2
$ echo 'package p; import "gopkg.in/russross/blackfriday.v2"; var _ blackfriday.Node' | goimports
package p

import blackfriday "gopkg.in/russross/blackfriday.v2"

var _ blackfriday.Node
$ 

Why did goimports rename this import? It should not be doing that. The package clause in the package already says blackfriday, and furthermore the package name follows the standard import path rules for package names on gopkg.in.

/cc @heschik @bradfitz

@gopherbot gopherbot added this to the Unreleased milestone Jan 4, 2019
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 4, 2019

This was was intentional. See #28428 and https://go-review.googlesource.com/c/152000

Closing, but feel free to reopen one of these bugs if you strongly disagree.

@bradfitz bradfitz closed this Jan 4, 2019
@heschik
Copy link
Contributor

@heschik heschik commented Jan 4, 2019

Yeah. goimports only understands SIV-style /vN suffixes and adds names to everything it finds surprising. It could be taught the gopkg.in convention, I suppose, but I'd like to have a coherent policy about what it should and shouldn't understand, per the discussion in #28428 (comment).

@heschik
Copy link
Contributor

@heschik heschik commented Jan 4, 2019

Hm. I'd forgotten that gopkg.in is already specially handled by the go command.

As a special case, module paths beginning with gopkg.in/ continue to use the
conventions established on that system: the major version is always present,
and it is preceded by a dot instead of a slash: gopkg.in/yaml.v1
and gopkg.in/yaml.v2, not gopkg.in/yaml and gopkg.in/yaml/v2.

That's some justification to support it in goimports, I suppose.

@rsc rsc reopened this Jan 7, 2019
@rsc
Copy link
Contributor Author

@rsc rsc commented Jan 7, 2019

As @heschik pointed out, gopkg.in is a known special case in the go command, so I agree that it should be a known special case here. It doesn't make sense to penalize gopkg.in users by renaming all their imports unnecessarily.

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 10, 2019

Change https://golang.org/cl/157357 mentions this issue: imports: drop anyting after . in package names

ianthehat added a commit to ianthehat/tools that referenced this issue Jan 11, 2019
A pacakge name cannot contain a . anyway, so this is a mostly likely
internal package name in the presence of a . in the import path.
This stops goimports from adding a local alias in places where it is not
needed, for instance in gopkg.in conventions.

Fixes golang/go#29556

Change-Id: I0ab11f2852d7f1dae14457995692760077201c8e
ianthehat added a commit to ianthehat/tools that referenced this issue Jan 28, 2019
A pacakge name cannot contain a . anyway, so this is a mostly likely
internal package name in the presence of a . in the import path.
This stops goimports from adding a local alias in places where it is not
needed, for instance in gopkg.in conventions.

Fixes golang/go#29556

Change-Id: I0ab11f2852d7f1dae14457995692760077201c8e
@golang golang locked and limited conversation to collaborators Jan 30, 2020
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.