Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Adapt goimports' fast filepath.Walk impl for ListPackages #419

Closed
fabulous-gopher opened this issue Apr 21, 2017 · 11 comments
Closed

Adapt goimports' fast filepath.Walk impl for ListPackages #419

fabulous-gopher opened this issue Apr 21, 2017 · 11 comments

Comments

@fabulous-gopher
Copy link

From @sdboyer on March 7, 2017 14:44

goimports has an optimized implementation of filepath.Walk() that we should adapt for ListPackages().

Copied from original issue: sdboyer/gps#180

@fabulous-gopher
Copy link
Author

From @jstemmer on April 1, 2017 22:19

I've had a quick look at the fastWalk implementation and it looks like it should be pretty easy to replace the current filepath.Walk implementation with this faster one. Some quick testing shows a consistent improvement in ListPackages performance, I can clean it up and prepare a PR.

Can we just copy the source into this repo with the licensing headers intact or is there something else (license-wise) that I should keep in mind?

@fabulous-gopher
Copy link
Author

From @sdboyer on April 1, 2017 23:38

Ugh, licenses. We're already trying to figure that out in the reverse direction - #300. Maybe it's best to just wait until after we finish that, as the licenses should then be compatible.

Also, a note - we may need it to be somewhat different from goimports' version, as our symlink needs may be a bit different - see #157 and #177.

@fabulous-gopher
Copy link
Author

From @jstemmer on April 1, 2017 23:48

Sure, I'll hold off on sending it for now and will keep it around in a local branch until gps is merged into dep.

I was vaguely aware that symlinks were being discussed in a couple of places but tried to avoid those discussions so far. I'll have a look through those issues and see what needs to be changed, thanks for linking them.

@fabulous-gopher
Copy link
Author

From @sdboyer on April 2, 2017 1:21

Sure, I'll hold off on sending it for now and will keep it around in a local branch until gps is merged into dep.

SGTM

I was vaguely aware that symlinks were being discussed in a couple of places but tried to avoid those discussions so far.

That seems like a good act of self-care. I am truly coming to despise symlinks.

@sdboyer
Copy link
Member

sdboyer commented May 3, 2017

@jstemmer ok, this one should be ready for your attention now 😄

@jstemmer
Copy link
Contributor

jstemmer commented May 3, 2017

Great! I'll move my branch over soon and see what else was left to be done wrt symlinks before sending a PR.

@sdboyer
Copy link
Member

sdboyer commented May 3, 2017

@jstemmer i think a direct merge may not be possible because there was a filter-branch op that changed commit history...but either way, shouldn't be too bad. thanks!

@sdboyer
Copy link
Member

sdboyer commented Aug 15, 2017

@jstemmer pinging, just to see if you maybe have time to look at this again 😄

@jstemmer
Copy link
Contributor

I've been busy lately, but I'll try to find some time to pick this up soon.

@sdboyer
Copy link
Member

sdboyer commented Aug 28, 2017

/cc @karrick, re: godirwalk

@karrick
Copy link
Contributor

karrick commented Aug 29, 2017

I have a tested branch of dep that uses https://github.com/karrick/godirwalk in ListPackages. After #1084 is merged, I'll submit a PR for this issue as well.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants