Skip to content
This repository has been archived by the owner on Feb 3, 2018. It is now read-only.

Is there a reason to parse go-import via own code instead of go/vcs package? #167

Closed
dmitshur opened this issue Feb 2, 2017 · 3 comments

Comments

@dmitshur
Copy link

dmitshur commented Feb 2, 2017

This is a question to help me understand something.

In golang/go#18387, @josharian mentioned:

Another partial copy of this bit of cmd/go can be found at https://github.com/sdboyer/gps (e.g. https://github.com/sdboyer/gps/blob/master/discovery.go).

That's how I came to notice that gps appears to implement some code to parse go-import meta information, especially in discovery.go. I see a comment there:

// This code is taken from cmd/go/discovery.go; it is the logic go get itself
// uses to interpret meta imports information.

My question is, are you aware of vcs.RepoRootForImportPath from golang.org/x/tools/go/vcs package, which exposes that functionality from cmd/go as an importable library? Have you considered using it? In particular, it seems to me that parseMetadata and all the code it uses can all be replaced in favor of calling vcs.RepoRootForImportPath. If you decided against using it, what was the rationale?

@sdboyer
Copy link
Owner

sdboyer commented Feb 2, 2017

Totally reasonable question.

While I think I was aware of those functions, honestly, I'm not sure. My memory is murky in part because while I was hunting around for example code to work from, I didn't have the exact requirements fully clear in my own mind. So I didn't have a clear basis for evaluating what was out there.

In any case, looking again now, it seems that those functions get most of my requirements right, but maybe not quite all of them:

  • I interleave not just per-vcs type, but potentially also per-domain control into the deduction engine
  • I wanted scheme determination to happen at a separate point in the algorithm
  • I did want at least the option to add more custom deduction logic (despite the gnarly inherent problems there)

It feels like there's also some other bits in SourceMgr.deduceFromPath() (which is more the equivalent to vcs.RepoRootForImportPath() than parseMetadata()) that differ, but they're not coming to mind.

That said, if we can swap in what's in the go/vcs repo for what I have there without changing behavior, I'm always happy to remove code :)

@dmitshur
Copy link
Author

dmitshur commented Feb 2, 2017

Got it, thanks. 👍

@sdboyer
Copy link
Owner

sdboyer commented Feb 2, 2017

Gonna close this out. If somebody wants to look at reimplementing what's there using the logic @shurcooL mentioned, say so and I'll reopen :)

@sdboyer sdboyer closed this as completed Feb 2, 2017
@sdboyer sdboyer mentioned this issue Mar 29, 2017
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants