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

Add Release() to the SourceManager interface #206

Merged
merged 5 commits into from
Apr 10, 2017
Merged

Add Release() to the SourceManager interface #206

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c379ad8
Add Release() to the SourceManager interface
spenczar Apr 2, 2017
7529448
Remove Release method from sourceBridge interface
spenczar Apr 10, 2017
9d6d587
Just copy the method set of SourceManager into sourceBridge
spenczar Apr 10, 2017
b1a499f
Merge branch 'master' into release_source_manager
spenczar Apr 10, 2017
c240c2a
Update method set of bridge to match SourceManager
spenczar Apr 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bridge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
// sourceBridges provide an adapter to SourceManagers that tailor operations
// for a single solve run.
type sourceBridge interface {
SourceManager // composes SourceManager
sourceDeducer
verifyRootDir(path string) error
pairRevision(id ProjectIdentifier, r Revision) []Version
pairVersion(id ProjectIdentifier, v UnpairedVersion) PairedVersion
Expand Down
12 changes: 12 additions & 0 deletions source_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,18 @@ var sanitizer = strings.NewReplacer(":", "-", "/", "-", "+", "-")
// sufficient for any purpose. It provides some additional semantics around the
// methods defined here.
type SourceManager interface {
// SourceManager adds a Release method to the sourceDeducer method set. The
// sourceDeducer method set is defined in a separate interface because it's
// used by the sourceBridge interface as well, but sourceBridges don't get a
// Release method.
sourceDeducer
Copy link
Owner

Choose a reason for hiding this comment

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

I was wondering to myself if this was the right way to do it while typing my earlier comment. If we do this, then the SourceManager interface in godoc will no longer have all those methods visible, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uuurgh yes, that's correct. I guess SourceDeducer should be exported.

Copy link
Owner

@sdboyer sdboyer Apr 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Deducer doesn't really cover it either, though, and we also use that name elsewhere.

My expectation with #159 is actually to depart pretty hard from the SourceManager interface with the *bridge, as that'll add a bunch of context.Context to the methods that there is just no good reason to have on the *bridge.

We can debate the reasons for that over in the other issue when time comes, but for now, it suggests to me that we can leave the public-facing interface here largely unchanged and instead just enumerate the SourceManager methods, except for Release(), directly in sourceBridge.

(that makes sense in my head, does it make sense written out?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, that works for me. Kind of a shame to have the methods written in two places, but if you want them to not use contexts in the bridge, it makes sense I suppose.

Kind of an aside, but gps might benefit from some renaming of some these interfaces, maybe breaking them into composable chunks a bit. I don't have anything too concrete, but names like "manager" and "bridge" and "analyzer" and "deducer" are all pretty similar.

It would probably be way easier to do that golang/dep#300 is solved.

Copy link
Owner

Choose a reason for hiding this comment

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

Yep, to all of the above. (Of those four, I hate bridge the most - it's not even the right pattern name. Though I think Deducer is OK).

// Release lets go of any locks held by the SourceManager. Once called, it is
// no longer safe to call methods against it; all method calls will
// immediately result in errors.
Release()
}

type sourceDeducer interface {
// SourceExists checks if a repository exists, either upstream or in the
// SourceManager's central repository cache.
SourceExists(ProjectIdentifier) (bool, error)
Expand Down