Fixes #316 Replace full path for vendor packages with relative path #491
Conversation
ramya-rao-a
force-pushed
the
vendor-imports
branch
from
e390d0c to
291e3ed
Compare
| return importedPkgs.indexOf(element) === -1; | ||
| }); | ||
| } | ||
| return pkgs.sort().slice(1); |
There was a problem hiding this comment.
Looks like this is removing a "" entry - do you know why that exists? Should this slice be a safer check? Maybe roll it into the filter?
There was a problem hiding this comment.
the stdout from gopkgs is a \n separated pkg paths. So when you do a split, you end up with a trailing empty string. That's what this originally was, and I just moved the code around while refactoring. Will fix it nevertheless.
| // If yes, then vendor pkg can be replaced with its relative path to the "vendor" folder | ||
| if (vendorIndex > 0) { | ||
| let rootProjectForVendorPkg = path.join(currentWorkspace, pkg.substr(0, vendorIndex)); | ||
| let relativePathForVendorPkg = pkg.substring(vendorIndex + 8); |
There was a problem hiding this comment.
yup, will pull out '/vendor/' into a var, so use .length so that 8 is not mysterious
| // The current project github.com/someotherrepo/b may have a version of a as a vendor package | ||
| // github.com/someotherrepo/b/vendor/github.com/somerepo/a | ||
| // In this case, github.com/somerepo/a would already be returned by goPkgs | ||
| if (pkgs.indexOf(relativePathForVendorPkg) === -1) { |
There was a problem hiding this comment.
Would it be worth it to get rid of this check for perf? It's only applied to vendored packages so maybe it's not a big deal but do people have 100s of vendored packages? You could make pkgs a Set, or do other things to de-dupe.
There was a problem hiding this comment.
set is good idea, though in the end I'll have to convert it to array to have sorting
|
This seems pretty straightforward, so it doesn't seem like the delay to start up govendor is worth it? |
| return; | ||
| } | ||
|
|
||
| let vendorIndex = pkg.indexOf('/vendor/'); |
There was a problem hiding this comment.
Can packages not include '/vendor/' somewhere else in the name? Could I 'go get github.com/foo/vendor' then this might be a false positive.
There was a problem hiding this comment.
Below is from Understanding and using the vendor folder
However, if a package or a parent folder of a package contains folder named vendor it will be searched for dependencies using the vendor folder as an import path root.
And while trying different combinations, I could confirm that by "parent", they mean "ancestor"
Since that is the exact check I make which is making sure that the "vendor" folder's ancestors are current package's ancestors too, we should be good.
Go doesn't stop you from creating a package by the name "vendor", so you could totally do a "go get github.com/foo/vendor". Any packages under this folder will be available for use to any Go projects under "github.com/foo/*"
ramya-rao-a
force-pushed
the
vendor-imports
branch
from
ddfa63e to
8f5bcf1
Compare
ramya-rao-a
force-pushed
the
vendor-imports
branch
from
8f5bcf1 to
1ae7c5c
Compare
| @@ -9,31 +9,91 @@ import vscode = require('vscode'); | |||
| import cp = require('child_process'); | |||
PR to fix #316
This PR uses the prefix from the full path of vendor packages, appends that to $GOPATH to get the path to the vendor folder, tries to match it with current file path to see if both belong to the same project. If yes, then the relative path of the vendor replaces the full path.
Another way to find relative paths for vendor packages is by using the govendor tool:
govendor list -no-status +vgives the list of vendor packages (with relative path) for current go project.But using this adds a 1 second delay between executing the command
Go: Add Importand showing the results in Windows. In Mac, the difference is much less.The implementation using govendor is here: https://github.com/ramya-rao-a/vscode-go/blob/vendor-imports-using-govendor/src/goImport.ts
Thoughts on both approaches are welcome.