Use gps.SourceManager instead of *gps.SourceMgr in cmd/dep #353

spenczar · 2017-03-28T14:19:36Z

This is a refactor that shouldn't change behavior. It just gets package main to use interfaces instead of concrete implementations for SourceManager.

The idea here is to make it easier to write tests that mock the source manager's behavior. I have some tests of the status command I'd like to write to get it to support the required field.

spenczar · 2017-03-28T14:21:04Z

cmd/dep/signals.go

@@ -0,0 +1,23 @@
+package main


I'm not very certain about this file. HandleSignals and Release are methods on *SourceMgr that aren't defined in the SourceManager interface, but which are used by commands. They seem pretty concrete, so I figured these interface assertions might be an okay way forward. I'm interested in feedback.

Yeah, I was sloppy with these bits. OK, so...

Release() is something that should be promoted into the SourceManager interface. I'm fine with the idea that any SourceManager should necessarily have a lifecycle. There's no way they can do the job promised by the interface without doing state management (unless they're just mocks), so there's no sense in omitting lifecycle methods from the interface.

HandleSignals is something I'm still shifting on, though. Per #160, having the library set up and do the signal handling itself isn't exactly ideal, even if it is triggered by the caller.

I'd move Release() into the interface, but the interface lives in gps. Maybe this should just wait for #300 to get resolved? I'm not sure.

Nah, we can just do it as a separate PR against gps right now. No problem there.

spenczar · 2017-03-28T14:22:55Z

context.go

+	VersionInWorkspace(root gps.ProjectRoot) (gps.Version, error)
+
+	// Returns the root directory where packages can be found.
+	Root() string


I'm extreeeeeemely uncertain about this method. cmd/dep/init.go wanted to use ctx.GOPATH directly, but I'm not really certain why. It seemed like it was trying to find the local filepath to a project in GOPATH. Maybe there's a new method we could add to BuildContext that better encapsulates that use case.

Eh...I could be convinced of one for facilitating searching for directories, but I don't think having this Root() method adds much value. We don't want to rename this away from GOPATH, because that's what people already think of wrt "a space on disk where your source packages are kept"; a new name would just add confusion.

Yep, I agree. I was trying to do the minimal change for the first set to submit for PR.

I think some sort of search-gopath-for-a-project method seems right.

sdboyer

In general, I think this is a worthwhile abstraction. But there's a lot of details here to work out, first.

Rather than going forward iteratively in a PR, could you open an issue to make a proposal about what the scope of responsibilities should be here?

sdboyer · 2017-04-01T01:00:49Z

context.go

@@ -16,14 +16,41 @@ import (
 	"github.com/sdboyer/gps"
 )

+type BuildContext interface {


I like the principle here, but IMO DepContext is preferable to BuildContext, as we're not really building anything here.

Yep, BuildContext doesn't seem right. I don't love how DepContext stutters the package name (dep.DepContext). Is just Context okay? That's also pretty vague.

Most of the operations here are filesystem operations, apart from SourceManager(). Is there something we could call it related to that? I'm not sure.

Yeah, I'm not a fan of that aspect, either. I considered plain Context; while it is what go/build does, that was also before the context package became a thing. Seems like it dominates that naming now, and I didn't have an alternative, sooo... 😄

sdboyer · 2017-04-01T01:07:41Z

cmd/dep/signals.go

@@ -0,0 +1,23 @@
+package main


Yeah, I was sloppy with these bits. OK, so...

Release() is something that should be promoted into the SourceManager interface. I'm fine with the idea that any SourceManager should necessarily have a lifecycle. There's no way they can do the job promised by the interface without doing state management (unless they're just mocks), so there's no sense in omitting lifecycle methods from the interface.

HandleSignals is something I'm still shifting on, though. Per #160, having the library set up and do the signal handling itself isn't exactly ideal, even if it is triggered by the caller.

sdboyer · 2017-04-01T01:15:16Z

context.go

+	VersionInWorkspace(root gps.ProjectRoot) (gps.Version, error)
+
+	// Returns the root directory where packages can be found.
+	Root() string


Eh...I could be convinced of one for facilitating searching for directories, but I don't think having this Root() method adds much value. We don't want to rename this away from GOPATH, because that's what people already think of wrt "a space on disk where your source packages are kept"; a new name would just add confusion.

spenczar · 2017-04-01T04:15:58Z

In general, I think this is a worthwhile abstraction. But there's a lot of details here to work out, first.

Rather than going forward iteratively in a PR, could you open an issue to make a proposal about what the scope of responsibilities should be here?

BuildContext wasn't really what I hoped to focus on. My hope was to eventually write tests of the Status command, which would be easier if I could mock interfaces, so I wanted to use gps.SourceManager instead of *gps.SourceMgr. I happened to change *Ctx to a new interface too just because the *Ctx was the thing that made the Source Manager.

I think I can get away with writing those tests even with a concrete *Ctx though. I'll revert the changes to context and focus on just the SourceManager aspect. Does that sound OK?

sdboyer · 2017-04-01T04:26:41Z

I think I can get away with writing those tests even with a concrete *Ctx though. I'll revert the changes to context and focus on just the SourceManager aspect. Does that sound OK?

I do think they're a valuable direction to explore, but if they're yaks vis-a-vis your current goals, then sure 😄

sdboyer · 2017-04-17T01:58:57Z

@spenczar did you want to keep trying to move this forward?

spenczar · 2017-04-17T13:20:39Z

Oops, thanks for the reminder. Let's drop this and do it after gps is merged into dep, it'll be much easier then.

…

On Sun, Apr 16, 2017 at 9:58 PM sam boyer ***@***.***> wrote: @spenczar <https://github.com/spenczar> did you want to keep trying to move this forward? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#353 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA1vHcN1-hEzKEBo5745462fC9RFa2oAks5rwsdjgaJpZM4MrsNo> .

sdboyer · 2017-04-17T19:16:23Z

SGTM

sdboyer · 2017-05-05T13:53:23Z

Is it time to pick this back up?

spenczar · 2017-05-06T15:20:50Z

Yes! I'll dig in.

spenczar · 2017-05-06T16:04:28Z

I've pushed a new version which only changes *SourceMgr references to SourceManager. The build context stuff takes a little more design, so it can be done separately with some more discussion.

sdboyer · 2017-05-09T00:17:12Z

Yeah OK, this is a good division of work, I think.

Feel free with that follow-up at your leisure 😄

Use gps.SourceManager instead of *gps.SourceMgr in cmd/dep

spenczar commented Mar 28, 2017

View reviewed changes

sdboyer suggested changes Apr 1, 2017

View reviewed changes

googlebot added the cla: yes label Apr 1, 2017

spenczar mentioned this pull request Apr 2, 2017

Add Release() to the SourceManager interface sdboyer/gps#206

Merged

Use gps.SourceManager instead of *gps.SourceMgr

25a5016

spenczar force-pushed the cmd_use_interfaces branch from 7bb4c77 to 25a5016 Compare May 6, 2017 16:00

spenczar changed the title ~~Use interfaces between main and dep packages~~ Use gps.SourceManager instead of *gps.SourceMgr in cmd/dep May 6, 2017

sdboyer approved these changes May 9, 2017

View reviewed changes

sdboyer merged commit 4c5e459 into golang:master May 9, 2017

ibrasho pushed a commit to ibrasho-forks/dep that referenced this pull request May 10, 2017

Merge pull request golang#353 from spenczar/cmd_use_interfaces

c3d0bf2

Use gps.SourceManager instead of *gps.SourceMgr in cmd/dep

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use gps.SourceManager instead of *gps.SourceMgr in cmd/dep #353

Use gps.SourceManager instead of *gps.SourceMgr in cmd/dep #353

spenczar commented Mar 28, 2017 •

edited

Loading

spenczar Mar 28, 2017

sdboyer Apr 1, 2017

spenczar Apr 1, 2017

sdboyer Apr 1, 2017

spenczar Mar 28, 2017

sdboyer Apr 1, 2017

spenczar Apr 1, 2017

sdboyer left a comment

sdboyer Apr 1, 2017

spenczar Apr 1, 2017

sdboyer Apr 1, 2017

sdboyer Apr 1, 2017

sdboyer Apr 1, 2017

spenczar commented Apr 1, 2017

sdboyer commented Apr 1, 2017

sdboyer commented Apr 17, 2017

spenczar commented Apr 17, 2017 via email

sdboyer commented Apr 17, 2017

sdboyer commented May 5, 2017

spenczar commented May 6, 2017

spenczar commented May 6, 2017

sdboyer commented May 9, 2017

Use gps.SourceManager instead of *gps.SourceMgr in cmd/dep #353

Use gps.SourceManager instead of *gps.SourceMgr in cmd/dep #353

Conversation

spenczar commented Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spenczar commented Apr 1, 2017

sdboyer commented Apr 1, 2017

sdboyer commented Apr 17, 2017

spenczar commented Apr 17, 2017 via email

sdboyer commented Apr 17, 2017

sdboyer commented May 5, 2017

spenczar commented May 6, 2017

spenczar commented May 6, 2017

sdboyer commented May 9, 2017

spenczar commented Mar 28, 2017 •

edited

Loading