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: more feature-rich ignore file support #16417

Closed
tj opened this issue Jul 18, 2016 · 11 comments
Closed

x/tools/cmd/goimports: more feature-rich ignore file support #16417

tj opened this issue Jul 18, 2016 · 11 comments
Labels
FrozenDueToAge Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@tj
Copy link

tj commented Jul 18, 2016

Please answer these questions before submitting your issue. Thanks!

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

go version go1.6.2 darwin/amd64

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

OSX

  1. What did you do?
    If possible, provide a recipe for reproducing the error.
    A complete runnable program is good.
    A link on play.golang.org is best.

I use go get for non-Go packages as well, however this leads to many Node/client-side projects which have node_modules, and thus hundreds of thousands of directories.

  1. What did you expect to see?

I'd love to add node_modules or similar into .goimportsignore and bypass all of those easily.

  1. What did you see instead?
...
2016/07/18 15:58:28.204027 scanning dir /Users/tj/dev/src/github.com/tj/d3-pie/node_modules/tty-browserify
2016/07/18 15:58:28.204357 scanning dir /Users/tj/dev/src/github.com/tj/d3-line/node_modules/semver-regex
2016/07/18 15:58:28.204378 scanning dir /Users/tj/dev/src/github.com/tj/d3-pie/node_modules/tunnel-agent
2016/07/18 15:58:28.204399 scanning dir /Users/tj/dev/src/github.com/tj/d3-series/node_modules/base64id
2016/07/18 15:58:28.204424 scanning dir /Users/tj/dev/src/github.com/tj/d3-pie/node_modules/read-pkg-up
2016/07/18 15:58:28.204443 scanning dir /Users/tj/dev/src/github.com/tj/d3-pie/node_modules/tweetnacl
2016/07/18 15:58:28.204466 scanning dir /Users/tj/dev/src/github.com/tj/d3-donut/node_modules/yallist
2016/07/18 15:58:28.204505 scanning dir /Users/tj/dev/src/github.com/tj/d3-pie/node_modules/stringstream
2016/07/18 15:58:28.204526 scanning dir /Users/tj/dev/src/github.com/tj/d3-pie/node_modules/punycode
...

for days. Arguably not really a goimports problem since I'm mis-using GOPATH, but I know quite a few people who do similar.

@bradfitz
Copy link
Contributor

Is it plausible for somebody to have *.go files under node_modules?

If not, maybe we can just blacklist that name entirely, without configuration. We already blacklist some names (".git", ".hg", etc).

Otherwise more configuration seems fine.

@bradfitz bradfitz added this to the Unreleased milestone Jul 18, 2016
@tj
Copy link
Author

tj commented Jul 18, 2016

Not in my case nope! Unless someone is trying to be clever using NPM to install Go code I can't see anyone doing it, should be fine to blacklist.

@gopherbot
Copy link

CL https://golang.org/cl/25044 mentions this issue.

gopherbot pushed a commit to golang/tools that referenced this issue Jul 19, 2016
Updates golang/go#16417

Change-Id: Ia4a5f997036274d09cca2ff10e500e403c1725ba
Reviewed-on: https://go-review.googlesource.com/25044
Reviewed-by: Andrew Gerrand <adg@golang.org>
@pi
Copy link

pi commented Jul 19, 2016

I think wildcards will solve problems of this kind in the future.
.goimportsignore
*/node_modules/*

@bradfitz
Copy link
Contributor

@pi, indeed. But I was hoping to defer implementing that until there's enough evidence that it's really needed. It's more work for users and more complexity to document, and matching globs takes more time. If we can get 90% of the way there automatically by hard-coding all the common cases, it's easier and faster for everyone. So let's wait and see.

The "node_modules" thing has been committed.

@tj
Copy link
Author

tj commented Jul 19, 2016

Oh man, ~300ms now even with my .goimportsremoved removed, I guess that was the source of all pain for me haha thanks!

@bradfitz
Copy link
Contributor

300ms is still noticeable. Where is the time going? What OS and filesystem type? SSD? Under 200ms would be nicer.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@heschi
Copy link
Contributor

heschi commented Nov 7, 2019

Given the lack of activity here, closing.

@heschi heschi closed this as completed Nov 7, 2019
@ashi009
Copy link

ashi009 commented Aug 5, 2020

Any chance of reopening this, we really want to have a way to exclude bazel-*. #35914 is more or less blocked on this.

@heschi
Copy link
Contributor

heschi commented Aug 5, 2020

#35914 is blocked on someone coming up with a well-thought-out design for how the feature should work, not this bug.

@adam-azarchs
Copy link
Contributor

Yes, bazel-* is problematic because

  1. Those directories get regenerated by bazel at various times so just dropping a go.mod file into them isn't practical.
  2. The bazel-$(basename $PWD) directory is particularly problematic because it will include symlinks to the sources of the repository and also (in external) all of the remote repos, so goimports will absolutely find relevant import candidates. They'll just be completely incorrect.

I think the solution there, though, is that goimports shouldn't follow symlinks. go mod and go build don't, after all.

@golang golang locked and limited conversation to collaborators Apr 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

7 participants