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 · 10 comments
Closed

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

tj opened this issue Jul 18, 2016 · 10 comments
Labels
Milestone

Comments

@tj
Copy link

@tj 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

@bradfitz bradfitz commented Jul 18, 2016

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

@gopherbot gopherbot commented Jul 19, 2016

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 pi commented Jul 19, 2016

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

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jul 19, 2016

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

@bradfitz bradfitz commented Jul 19, 2016

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

@heschik
Copy link
Contributor

@heschik heschik commented Nov 7, 2019

Given the lack of activity here, closing.

@heschik heschik closed this Nov 7, 2019
@ashi009
Copy link

@ashi009 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.

@heschik
Copy link
Contributor

@heschik heschik 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.