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

Remove duplicate 'isStdLib' function #606

Merged
merged 6 commits into from
May 24, 2017
Merged

Remove duplicate 'isStdLib' function #606

merged 6 commits into from
May 24, 2017

Conversation

jmank88
Copy link
Collaborator

@jmank88 jmank88 commented May 19, 2017

cmd/dep has a function isStdLib which duplicates paths.IsStandardImportPath.

This change moves package paths up to the top level internal package, in order to allow use from cmd/dep, and removes isStdLib (after borrowing the more verbose comment).

Copy link
Member

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather keep gps as sealed-up as possible, for as long as possible. So, instead of moving this to internal/paths, let's just move it out of the nested internal - internal/gps/paths.

When we get around to re-exporting gps later, we can decide what to do about making it public.

@jmank88
Copy link
Collaborator Author

jmank88 commented May 19, 2017

I'm struggling to see the difference. Can you expand on what "as sealed-up as possible" means regarding package structure?

Has there been discussion about the logistics of re-exporting? Do you expect to move the whole gps subtree at once? Or just individual members?

@jmank88
Copy link
Collaborator Author

jmank88 commented May 19, 2017

Another option would be to drop package paths and just put the function into package gps.

@sdboyer
Copy link
Member

sdboyer commented May 19, 2017

I'm struggling to see the difference. Can you expand on what "as sealed-up as possible" means regarding package structure?

When gps was at github.com/sdboyer/gps, it necessarily had to function as a standalone system, without any "reverse" dependencies on dep. One of the ways that I keep responsibilities straight in my head is by maintaining that line, even after its relocation.

So - functionally, there's no difference. But for now, I think that keeping the subtree sealed - that is, not importing anything from dep outside its own subtree - imposes a discipline on the project that ultimately serves us well. Maybe we change that down the line, but I'd like that to be an intentional plan, not something we back our way into incrementally.

Has there been discussion about the logistics of re-exporting?

No actual discussion yet, but #575 is where it'll be.

Do you expect to move the whole gps subtree at once? Or just individual members?

My expectation is the whole subtree.

Another option would be to drop package paths and just put the function into package gps.

Ah, yes! This would probably be even better. The only reason to protect the identifier via internal the first place was when it was a var, rather than an actual function. The other refactoring you did relieved us of that requirement.

@jmank88
Copy link
Collaborator Author

jmank88 commented May 19, 2017

The only reason to protect the identifier via internal the first place was when it was a var, rather than an actual function.

Ah, that was the missing piece for me. I was thinking we still wanted to hide it one way or another.

package gps/pkgtree actually imports paths, so just dropping the function into gps would introduce a cycle. However, there is only a single usage...

func (rm ReachMap) FlattenOmitStdLib() []string {
	return rm.FlattenFn(paths.IsStandardImportPath)
}

...and now that the callers will have access to the function, they could just make the inline call themselves. This would also reduce the surface of the irregular Flatten* api, and generally decouple the two concepts.

@jmank88
Copy link
Collaborator Author

jmank88 commented May 19, 2017

Putting the function in gps actually introduces a cycle in pkgtree_test.go which can't easily be resolved via the usual package *_test due to accessing unexported package members. So we're back to gps/paths, but I still thought the Flatten change was sensible.

@sdboyer
Copy link
Member

sdboyer commented May 23, 2017

I think just the small conflict fix, then this should be ready.

@sdboyer
Copy link
Member

sdboyer commented May 23, 2017

Sorry, another conflict :/

@sdboyer sdboyer merged commit fe4b78c into golang:master May 24, 2017
@jmank88 jmank88 deleted the paths_pkg branch June 7, 2017 23:44
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