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

SplitAbsoluteProjectRoot re-name/doc #682

Merged
merged 1 commit into from
Jun 27, 2017
Merged

SplitAbsoluteProjectRoot re-name/doc #682

merged 1 commit into from
Jun 27, 2017

Conversation

jmank88
Copy link
Collaborator

@jmank88 jmank88 commented May 31, 2017

The doc for (and name of) SplitAbsoluteProjectRoot is stale (seems it used to check multiple GOPATHs and return the matched one). This change proposes updates, but it's exported api so let's get the name right. Anyone have other suggestions?
#672

@sdboyer
Copy link
Member

sdboyer commented May 31, 2017

This has definitely gotten stale. But, the name should probably include "ProjectRoot" somehow, as that's what it's returning.

@jmank88
Copy link
Collaborator Author

jmank88 commented May 31, 2017

This has definitely gotten stale. But, the name should probably include "ProjectRoot" somehow, as that's what it's returning.

Should it return type gps.ProjectRoot?

Does that type or name suggest to callers that they can pass some sub-package and expect it to find the project root for them? I was tempted to name it something straightforward like TrimGOPATHSrc.

@sdboyer
Copy link
Member

sdboyer commented May 31, 2017

Should it return type gps.ProjectRoot?

Maybe? But, see below.

Does that type or name suggest to callers that they can pass some sub-package and expect it to find the project root for them? I was tempted to name it something straightforward like TrimGOPATHSrc.

It's more the reverse - the type is generally used as a parameter more than a return value, and is intended as a signal to callers that they must do the work to ensure that any value they pass in is, in fact, a string that adheres to gps.ProjectRoot's semantics.

The only place we use it publicly as a return value, IIRC, is from SourceManager.DeduceProjectRoot(). And the intent there is clear, inasmuch as the input is a string, and the output is a gps.ProjectRoot But I'm not sure about using it as a return type here, as there's no real guarantee that the value we get back from this func is a logical gps.ProjectRoot.

@jmank88
Copy link
Collaborator Author

jmank88 commented May 31, 2017

But I'm not sure about using it as a return type here, as there's no real guarantee that the value we get back from this func is a logical gps.ProjectRoot.

That's what I was getting at: Won't a name including ProjectRoot mislead users in the exact same way? The return is only a root import if the argument was a root path, and non-roots are not rejected.

What about something like ImportPath/ImportForAbs/ImportFromAbs?
Edit: Reverse method being named in #688

@sdboyer
Copy link
Member

sdboyer commented Jun 6, 2017

Sorry, my time is becoming more and more scarce...

Honestly, I don't have a good answer here. I'm content to go with what you think will explain it best.

@jmank88
Copy link
Collaborator Author

jmank88 commented Jun 7, 2017

Updated to ImportForAbs, which mirrors AbsForImport from #688

@jmank88
Copy link
Collaborator Author

jmank88 commented Jun 22, 2017

Rebased

@sdboyer sdboyer merged commit 4bfa359 into golang:master Jun 27, 2017
@jmank88 jmank88 deleted the split_absolute_project_root branch June 27, 2017 12:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants