Break up Ctx.VersionInWorkspace #688
Conversation
|
Let's wait until after #500 for this - there are significant changes pending in how this gets called. |
|
Rebased |
internal/gps/vcs_version.go
Outdated
| ) | ||
|
|
||
| // ProjectVersion returns the current project version for an absolute path. | ||
| func ProjectVersion(path string) (Version, error) { |
There was a problem hiding this comment.
So I'm generally OK with having this logic in gps, except that I think we should avoid suggesting that this is getting versions for projects. There's a not-implausible future in which it's possible that we'd allow versions to be a property of metadata within the file tree itself (akin to Cargo.toml). In that case, it'd be up to the tool to impose these semantics.
Instead, let's use a weaker, more functionally-accurate name for the func: VCSVersion
internal/gps/vcs_version.go
Outdated
|
|
||
| // ProjectVersion returns the current project version for an absolute path. | ||
| func ProjectVersion(path string) (Version, error) { | ||
| repo, err := vcs.NewRepo("", path) |
There was a problem hiding this comment.
IIRC we have a gps-internal version of this call that uses our decorated types; as long as this is moving into gps, we should probably use that.
There was a problem hiding this comment.
Were you thinking of something in that wraps up or delegates to vcs.NewRepo behind the scenes? The only current dep/gps usage is this one.
|
Bump |
internal/gps/vcs_version.go
Outdated
| ) | ||
|
|
||
| // VSCVersion returns the current project version for an absolute path. | ||
| func VSCVersion(path string) (Version, error) { |
internal/gps/vcs_version_test.go
Outdated
| rev: NewVersion("v0.8.0").Pair("645ef00459ed84a119197bfb8d8205042c6df63d"), // semver | ||
| checkout: true, | ||
| }, | ||
| "github.com/Sirupsen/logrus": { |
There was a problem hiding this comment.
nit: with all the case sensitivity issues this is causing, let's go with the new one - sirupsen
|
@jmank88 i'm ready to merge this, but before that let's use this as a guinea pig case for codeclimate - https://twitter.com/codeclimate/status/887756447437770753 i'm gonna push a dummy commit onto this PR so that they can sniff around with what's going on. |
|
Codeclimate appears to have run successfully, though it is still showing as in progress here. |
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
OK, we're good to go. thanks! 🎉 |
#672
The
VersionInWorkspacelogic is coupled toCtxas a method, although it only accessesCtx.absoluteProjectRoot().This change proposes dissolving
VersionInWorkspacein favor of a new func with the interesting logic (gps.ProjectVersion) and exportingabsoluteProjectRoot(currently namedAbsForImport, but should coordinate with #682 ).