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

Conversation

spenczar
Copy link
Contributor

@spenczar spenczar commented Apr 2, 2017

This is already being used by dep. We discussed it a little bit in golang/dep#353.

This is already being used by dep.
@codecov
Copy link

codecov bot commented Apr 2, 2017

Codecov Report

Merging #206 into master will increase coverage by 0.32%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
bridge.go 75.91% <ø> (ø) ⬆️
source_manager.go 90.35% <ø> (ø) ⬆️
deduce.go 76.49% <0%> (ø) ⬆️
pkgtree/pkgtree.go 85.07% <0%> (+3.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6db8155...c240c2a. Read the comment docs.

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() }
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@spenczar spenczar Apr 10, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

// 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

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).

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.
@sdboyer
Copy link
Owner

sdboyer commented Apr 10, 2017

......wat? (at the appveyor failure)

@spenczar
Copy link
Contributor Author

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.

@spenczar
Copy link
Contributor Author

Yep, that's what it is. Will fix.

@sdboyer
Copy link
Owner

sdboyer commented Apr 10, 2017

Oh, wait, right, because CircleCI is just testing the PR branch, not the merged result. smh...

@sdboyer sdboyer merged commit 9659997 into sdboyer:master Apr 10, 2017
@sdboyer
Copy link
Owner

sdboyer commented Apr 10, 2017

Thanks!

krisnova pushed a commit to krisnova/dep that referenced this pull request Apr 21, 2017
Add Release() to the SourceManager interface
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants