Add Release() to the SourceManager interface #206
Conversation
This is already being used by dep.
Codecov Report
@@ Coverage Diff @@
## master #206 +/- ##
=========================================
+ Coverage 77.98% 78.3% +0.32%
=========================================
Files 28 28
Lines 4020 4020
=========================================
+ Hits 3135 3148 +13
+ Misses 660 650 -10
+ Partials 225 222 -3
Continue to review full report at Codecov.
|
bridge.go
Outdated
| @@ -353,6 +353,8 @@ func (b *bridge) SyncSourceFor(id ProjectIdentifier) error { | |||
| return b.sm.SyncSourceFor(id) | |||
| } | |||
|
|
|||
| func (b *bridge) Release() { b.sm.Release() } | |||
There was a problem hiding this comment.
Let's have this panic, instead. This should never be called on a bridge; by definition, the bridge only exists for the duration of a solve run, and a solve run is never responsible for closing the source manager it's using. Having a panic here is the best way to make that explicit.
Also, we'll be able to back this out soon, because I plan to have fixing #159 include dropping the composition of SourceManager into sourceBridge.
There was a problem hiding this comment.
Sorry for the delay here, I'm back from vacation :)
I can do what you ask for here, but it makes me pretty uneasy. It sounds more like either *bridge shouldn't try to fulfill the SourceManager interface, or Release shouldn't be in the interface. Faking the implementation by dropping in a panic here makes things pretty hard to read for an outsider. I don't really know what you mean when you say that " by definition, the bridge only exists for the duration of a solve run" or "a solve run is never responsible for closing the source manager it's using" - those are expectations of invariants that might be better checked by the compiler than through panics? But either way, that's a lot of context knowledge to expect of readers of this code, which makes the interface kind of moot.
But it's your call, really! If #159 fixes this, a panic is definitely the expedient way forward.
There was a problem hiding this comment.
Yep, good points. Up until this issue, and #159, there wasn't a strong reason not to have *bridge implement SourceManager - but I've always been a bit dubious about it. That's why sourceBridge is a separate interface in the first place; it used to not compose SourceManager at all. This issue, and #159, are really the ones that expose the difference between what the bridge does and what a full-on SourceManager does - so, it's time to separate the interfaces.
Having it panic was at least in part a an attempt to avoid a merge conflict with #210 by removing the interface composition...but since that's stalled until I can find the version type leak, it'd be fine to just do the separation here - stop composing SourceManager in sourceBridge, and instead include just the subset of methods (everything except Release()) that we actually need on the bridge directly.
source_manager.go
Outdated
| // 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Uuurgh yes, that's correct. I guess SourceDeducer should be exported.
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
This makes the method sets redundant, but sourceBridge shouldn't have a Release() method because it only ever deals with a single solve run. We could have a private unexported interface covering the intersection of the method sets, but then we'd lose out on godoc. Repeating the methods is the least-bad of our options.
|
......wat? (at the appveyor failure) |
|
It must be testing a merge commit that it made which includes #196, or something? That's all I can think of... maybe I'll merge master down into my branch. |
|
Yep, that's what it is. Will fix. |
|
Oh, wait, right, because CircleCI is just testing the PR branch, not the merged result. smh... |
|
Thanks! |
Add Release() to the SourceManager interface
This is already being used by dep. We discussed it a little bit in golang/dep#353.