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: adds redundant aliases #30051

Closed
speedyhoon opened this issue Feb 1, 2019 · 4 comments
Closed

x/tools/cmd/goimports: adds redundant aliases #30051

speedyhoon opened this issue Feb 1, 2019 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@speedyhoon
Copy link

What version of Go are you using?

go1.11.1 windows/amd64 (Windows 10)

Does this issue reproduce with the latest release?

Yes

What did you do?

run goimports in the current directory.
goimports .

What did you expect to see?

package main

import "github.com/speedyhoon/v8"

func main() {
        v8.Str(nil, "")
}

What did you see instead?

package main

import v8 "github.com/speedyhoon/v8"

func main() {
        v8.Str(nil, "")
}

I named this package "v8", abbreviated from "validation". (okay the name might be a little short)
The package name seems to be colliding with assumptions around goimports matching a versioning pattern like: ^v[0-9]+$
However the alias in this case is redundant because it matches the package name exactly.
@gopherbot gopherbot added this to the Unreleased milestone Feb 1, 2019
@mvdan
Copy link
Member

mvdan commented Feb 1, 2019

/cc @heschik @matloob @rogpeppe @myitcv

@myitcv
Copy link
Member

myitcv commented Feb 1, 2019

@speedyhoon - aside from any potential bug with goimports, you're going to need to use a different package name here:

#28435 (comment)

Because github.com/speedyhoon/v8 is also the module path as well as the path to the package named v8.

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 6, 2019
@heschi
Copy link
Contributor

heschi commented Feb 8, 2019

This is working as intended. goimports is recording the fact that this is a package named v8, rather than version 8 of the module github.com/speedyhoon. That allows it to avoid reading the package from disk in most cases, speeding up the common case where no dependencies are missing. I don't think there's a way to fix this without sacrificing performance in the much more common case that /vN is a version suffix.

@heschi heschi closed this as completed Feb 8, 2019
@sjenning sjenning mentioned this issue Mar 13, 2019
Merged
@landism landism mentioned this issue Apr 18, 2019
Merged
@hex108 hex108 mentioned this issue Apr 28, 2019
Closed
@stevesloka stevesloka mentioned this issue May 8, 2019
Merged
@misterikkit
Copy link

That allows it to avoid reading the package from disk in most cases,

Sorry to jump in on a closed issue, but how/why does this improve performance? We have a ton of packages in kubernetes that use /vN to denote versions, and the redundant import alias is still annoying and unwanted.

@misterikkit misterikkit mentioned this issue Jun 4, 2019
Merged
@bertinatto bertinatto mentioned this issue Jul 11, 2019
Merged
@MarcelMue MarcelMue mentioned this issue Aug 1, 2019
Merged
@verb verb mentioned this issue Jan 15, 2020
Merged
@andrewlecuyer andrewlecuyer mentioned this issue Mar 16, 2020
Merged
6 tasks
@golang golang locked and limited conversation to collaborators May 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
7 participants
@andybons @misterikkit @myitcv @mvdan @gopherbot @speedyhoon @heschi