move deduceConstraint to SourceManager #761
Conversation
|
interesting! yeah, i think this fundamentally reasonable. i haven't properly reviewed, but probably need to spruce up docs, maybe change the method name, etc. i'll try to give this a deeper look soon. |
|
@sdboyer thanks! let me know and I'll follow up with an update |
internal/gps/solve_basic_test.go
Outdated
| @@ -1550,6 +1553,72 @@ func (sm *depspecSourceManager) ignore() map[string]bool { | |||
| return sm.ig | |||
| } | |||
|
|
|||
| // DeduceConstraint tries to puzzle out what kind of version is given in a string - | |||
| // semver, a revision, or as a fallback, a plain tag | |||
| func (sm *depspecSourceManager) DeduceConstraint(s string, pi ProjectIdentifier) (Constraint, error) { | |||
There was a problem hiding this comment.
FYI, I've made more changes to this function in #715. So whoever merges first, wins! 😉
internal/gps/solve_basic_test.go
Outdated
| @@ -1550,6 +1553,72 @@ func (sm *depspecSourceManager) ignore() map[string]bool { | |||
| return sm.ig | |||
| } | |||
|
|
|||
| // DeduceConstraint tries to puzzle out what kind of version is given in a string - | |||
| // semver, a revision, or as a fallback, a plain tag | |||
| func (sm *depspecSourceManager) DeduceConstraint(s string, pi ProjectIdentifier) (Constraint, error) { | |||
There was a problem hiding this comment.
This probably isn't the place to put a real implementation. The depspecSourceManager is specifically, and exclusively (at least for now) used to provide a SourceManager for solver tests, based on an input depspec. These depspecs do generally rely on semver, but they don't use "real" revisions or anything - just arbitrary strings, as version identifiers aren't the system under test.
In fact, the implementation here should probably be a panic, as there's no current circumstance under which the depspecSourceManager is useful outside of the gps solving tests, and it shouldn't be used anywhere else without a conscious and intentional expansion of its semantics. A panic would prevent inadvertent usage and scope creep.
| return c, nil | ||
| } | ||
|
|
||
| slen := len(s) |
There was a problem hiding this comment.
By moving this into *sourceMgr, we create an opportunity to do better validation here. Rather than deducing whether it's a revision from what general rules we have about the patterns of the bytes, we can actually hand this off to a sourceGateway, which can validate that it's an appropriate revision for that source type.
However, that may end up being somewhat involved to really do well, so we can defer it to a later issue. (Let's open up a follow-up prior to merging this issue for doing that, though).
There was a problem hiding this comment.
I've created #800 to track this.
internal/gps/source_manager.go
Outdated
| @@ -455,6 +462,72 @@ func (sm *SourceMgr) DeduceProjectRoot(ip string) (ProjectRoot, error) { | |||
| return ProjectRoot(pd.root), err | |||
| } | |||
|
|
|||
| // DeduceConstraint tries to puzzle out what kind of version is given in a string - | |||
| // semver, a revision, or as a fallback, a plain tag | |||
| func (sm *SourceMgr) DeduceConstraint(s string, pi ProjectIdentifier) (Constraint, error) { | |||
There was a problem hiding this comment.
Let's rename this to InferConstraint. This is especially important given the below plan about changing how we do revision handling; before we were doing some deduction and inference, but after we make those changes to revision handling, eventually it'll really just be inference.
marwan-at-work
force-pushed
the
move-deduce-constraint
branch
from
1f6e95f
to
6f32508
Compare
|
@sdboyer @carolynvs sorry took me a bit to get around this 🙈 -- but pr is updated! |
| return c, nil | ||
| } | ||
|
|
||
| slen := len(s) |
There was a problem hiding this comment.
I've created #800 to track this.
|
@carolynvs didn't think i would lol - see you at gophercon 🎉 |
What does this do / why do we need it?
Moves the function
deduceConstraintto the SourceManager interface.What should your reviewer look out for in this PR?
I think the
DeduceConstraintimplementation fordepspecSourceManagercould be simpler, since it's a mock implementation? not sureDo you need help or clarification on anything?
natm
Which issue(s) does this PR fix?
#654