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

Jump to bottom

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
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@rsc
Copy link
Contributor

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 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 as completed Jan 4, 2019
@heschi
Copy link
Contributor

heschi 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).

@heschi
Copy link
Contributor

heschi 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 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

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
@ianthehat
imports: drop anything after a non identifier rune in package names
63fd538 
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
@ianthehat
imports: drop anything after a non identifier rune in package names
7511e47 
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
@puellanivis puellanivis mentioned this issue Jan 31, 2019
Closed
@dpordomingo dpordomingo mentioned this issue Feb 8, 2019
Merged
@smacker smacker mentioned this issue Mar 6, 2019
Merged
@manno manno mentioned this issue Jul 12, 2019
Merged
@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.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@bradfitz @rsc @gopherbot @heschi