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: doesn't handle packages named "v1", etc #29041

Closed
mattmoor opened this issue Nov 30, 2018 · 10 comments

Comments

Projects
None yet
5 participants
@mattmoor
Copy link

commented Nov 30, 2018

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

$ go version
go version go1.11 linux/amd64

I'm seeing a bad format with goimports at commit: 1c3d964395ce8f04f3b03b30aaed0b096c08c3c6

You can see the bad formatting turned into a PR here: knative/pkg#176

The comment run is what's in the PR description (minus the -s, it's WIP).

cc @bradfitz

@gopherbot gopherbot added this to the Unreleased milestone Nov 30, 2018

@mattmoor

This comment has been minimized.

Copy link
Author

commented Nov 30, 2018

I see this with the head of release-branch.go1.11 as well.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

Don't name your packages v1, I guess?

@bradfitz bradfitz changed the title x/tools/goimports: strips necessary import x/tools/cmd/goimports: doesn't handle packages named "v1", etc Nov 30, 2018

@mattmoor

This comment has been minimized.

Copy link
Author

commented Nov 30, 2018

Yeah this blows up on anything touching K8s :P

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

Was this caused by https://go-review.googlesource.com/122616? (golang/tools@893c2b1)

Curious whether/how people think we should fix this.

@rsc @heschik @bcmills @dmitshur @kevinburke @Gnouc @josharian @mvdan @fraenkel @haya14busa

@bcmills

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

See also #28435.

@bcmills

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

Curious whether/how people think we should fix this.

If v1 is used as the Expr in a SelectorExpr, then goimports should not remove packages that end with the suffix v1. (It's fine if it doesn't add them, though: v1 is a terrible name for a package.)

We can compute the set of “names that might be package names” in an O(N) scan over the source file.

@bcmills

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

As a workaround, what happens if you rename the package explicitly on import?

That is, change

import (
	"k8s.io/api/core/v1"
)

to

import (
	v1 "k8s.io/api/core/v1"
)

?

@heschik

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2018

I haven't looked at this closely but I believe Bryan's suggestion will fix it, and my fix for #28428 will add the v1 automatically.

@heschik

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2018

@heschik

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

Confirmed that tip goimports now adds the v1 name to the import rather than deleting it.

@heschik heschik closed this Dec 5, 2018

negz added a commit to negz/crossplane that referenced this issue Mar 6, 2019

Import versioned ('v1' etc) style names explicitly
golang/go#29041

This is a slightly counterintuitive workaround for the above bug. The
alternative to this commit would be to disable goimports linting until the bug
is fixed. I think it's worth the workaround for the time being in exchange for
the value goimports linting brings.

Signed-off-by: Nic Cope <negz@rk0n.org>

negz added a commit to negz/crossplane that referenced this issue Mar 6, 2019

Import versioned ('v1' etc) style names explicitly
golang/go#29041

This is a slightly counterintuitive workaround for the above bug. The
alternative to this commit would be to disable goimports linting until the bug
is fixed. I think it's worth the workaround for the time being in exchange for
the value goimports linting brings.

Signed-off-by: Nic Cope <negz@rk0n.org>

negz added a commit to negz/crossplane that referenced this issue Mar 6, 2019

Import versioned ('v1' etc) style names explicitly
golang/go#29041

This is a slightly counterintuitive workaround for the above bug. The
alternative to this commit would be to disable goimports linting until the bug
is fixed. I think it's worth the workaround for the time being in exchange for
the value goimports linting brings.

Signed-off-by: Nic Cope <negz@rk0n.org>

negz added a commit to negz/crossplane that referenced this issue Mar 6, 2019

Import versioned ('v1' etc) style names explicitly
golang/go#29041

This is a slightly counterintuitive workaround for the above bug. The
alternative to this commit would be to disable goimports linting until the bug
is fixed. I think it's worth the workaround for the time being in exchange for
the value goimports linting brings.

Signed-off-by: Nic Cope <negz@rk0n.org>

negz added a commit to negz/crossplane that referenced this issue Mar 6, 2019

Import versioned ('v1' etc) style names explicitly
golang/go#29041

This is a slightly counterintuitive workaround for the above bug. The
alternative to this commit would be to disable goimports linting until the bug
is fixed. I think it's worth the workaround for the time being in exchange for
the value goimports linting brings.

Signed-off-by: Nic Cope <negz@rk0n.org>

negz added a commit to negz/crossplane that referenced this issue Mar 6, 2019

Import versioned ('v1' etc) style names explicitly
golang/go#29041

This is a slightly counterintuitive workaround for the above bug. The
alternative to this commit would be to disable goimports linting until the bug
is fixed. I think it's worth the workaround for the time being in exchange for
the value goimports linting brings.

Signed-off-by: Nic Cope <negz@rk0n.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.