Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

Update Masterminds/semver to latest 2.x #573

Closed
wants to merge 4 commits into from
Closed

Update Masterminds/semver to latest 2.x #573

wants to merge 4 commits into from

Conversation

nijaru
Copy link

@nijaru nijaru commented May 12, 2017

  1. Ran dep ensure -update
  2. Fixed build errors related to semver types in gps, including a return type in gps/manager_test.go
  3. The following tests are failing:

--- FAIL: TestVersionInWorkspace (1.36s)
context_test.go:169: expected "v0.8.0", got "v0.8.0"
--- FAIL: TestWriteManifest (0.00s)
manifest_test.go:94: Valid manifest did not marshal to TOML as expected:
(GOT): ignored = ["github.com/foo/bar"]

	[[dependencies]]
	  name = "github.com/babble/brook"
	  revision = "d05d5aca9f895d19e9265839bffeadd74a2d2ecb"
	
	[[dependencies]]
	  name = "github.com/golang/dep/internal/gps"
	  version = "^0.12.0"
	
	[[overrides]]
	  branch = "master"
	  name = "github.com/golang/dep/internal/gps"
	  source = "https://github.com/golang/dep/internal/gps"
	
		(WNT): ignored = ["github.com/foo/bar"]
	
	[[dependencies]]
	  name = "github.com/babble/brook"
	  revision = "d05d5aca9f895d19e9265839bffeadd74a2d2ecb"
	
	[[dependencies]]
	  name = "github.com/golang/dep/internal/gps"
	  version = ">=0.12.0, <1.0.0"
	
	[[overrides]]
	  branch = "master"
	  name = "github.com/golang/dep/internal/gps"
	  source = "https://github.com/golang/dep/internal/gps"

FAIL
exit status 1
FAIL github.com/golang/dep 10.427s


I believe this gets "[almost] the whole way there."

Going to look over this again later, but some guidance would be helpful.

Referencing #562

@ibrasho
Copy link
Collaborator

ibrasho commented May 13, 2017

Change semVersion.sv to type semver.Version instead of *semver.Version in internal/gps/version.go and reflect that in version.go and constraints.go` to fix the first test. I'm not sure what changed after the update, but the versions aren't being equal because the points are different.

In manifest_test.go: TestWriteManifest sets the constrant to gps.NewSemverConstraint("^v0.12.0") while in the TOML file it's ">=0.12.0, <1.0.0". Update the test to fix this issue. Just run go test -update to update the TOML files and update the TestReadManifest test case to the correct version "^0.12.0".

@nijaru
Copy link
Author

nijaru commented May 13, 2017

Thanks for the help! I updated the second test and version specified, and that is now passing.

I see what I did wrong with the first one. I will refactor that and see if I can get it working.

@googlebot
Copy link
Collaborator

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 cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

Copy link
Member

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

pretty close - just two things. so far it does appear that my fears of more complex problems were probably unfounded 😄

@@ -25,7 +25,7 @@ func TestReadManifest(t *testing.T) {
t.Fatalf("Should have read Manifest correctly, but got err %q", err)
}

c, _ := gps.NewSemverConstraint(">=0.12.0, <1.0.0")
c, _ := gps.NewSemverConstraint("^0.12.0")
Copy link
Member

Choose a reason for hiding this comment

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

I haven't absorbed the full context here yet, but just noting that these two aren't equivalent. By the definitions used in Masterminds/semver, ^0.12.0 is equivalent to >=0.12.0, <0.13.0.

Copy link
Author

Choose a reason for hiding this comment

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

Looking at the repo's readme, I believe those are equal. The change may not be necessary if the testdata is not updated, I'll test this out locally.

Copy link
Member

Choose a reason for hiding this comment

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

They're not 😄 Masterminds/semver#53

Copy link
Member

Choose a reason for hiding this comment

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

(note that updating the README there is one of the last TODOs before releasing v2.0.0)

Copy link
Author

Choose a reason for hiding this comment

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

Ah, that makes sense now :p

Gopkg.lock Outdated
@@ -28,7 +28,7 @@ memo = "932b7b1663f6eecccb1fada1d3670ae24cd8aa7c8b61e3b224edfefebe25954e"
branch = "master"
name = "github.com/pelletier/go-toml"
packages = ["."]
revision = "fe206efb84b2bc8e8cfafe6b4c1826622be969e3"
revision = "685a1f1cb7a66b9cadbe8f1ac49d9f8f567d6a9d"
Copy link
Member

Choose a reason for hiding this comment

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

let's only update the semver dep in this PR, please

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

Nick Russo added 4 commits May 14, 2017 14:53
@sdboyer
Copy link
Member

sdboyer commented May 18, 2017

Thanks for the contribution - I ended up merging the other PR related to this issue (#579), in part because the author there had claimed the issue. I realize that "issue claiming" isn't the most optimal way to do things, but we've got quite a lot of people interested in contributing, so I'm trying to respect it as a way of avoiding having people tripping over each other.

I hope you don't find this too discouraging, and are still interested in finding other issues to work on!

@sdboyer sdboyer closed this May 18, 2017
@nijaru
Copy link
Author

nijaru commented May 18, 2017

Not a problem at all, and I respect that.

I had some free time on Friday and I wanted to get my hands dirty either way. I'll keep my eyes peeled for more issues 😄

@sdboyer
Copy link
Member

sdboyer commented May 18, 2017

awesome 🎉 i appreciate your understanding! hopefully i'll be able to find time to whip some more issues into contribution shape soon...

@nijaru
Copy link
Author

nijaru commented May 18, 2017

Let me know if there's any way I can help with managing the project at all.

I was thinking I could work on documentation to help get people on board with using dep

@sdboyer
Copy link
Member

sdboyer commented May 18, 2017

Let me know if there's any way I can help with managing the project at all.

It's a tough thing to split up in general, but right now I have the feeling that there's probably a number of open issues that we could reasonably go back and mark as help-wanted.

Maybe we could take advantage of your being newish to dep: if you could go through the backlog of open issues and just look for issues (not already marked help-wanted) that, to your eyes, don't seem too crazy or daunting, but maybe just need some more info (or even seem approachable as-is). We could use that list to try to marshal up some more issues for contributors.

If that sounds like something you'd be interested in, it's probably best to coordinate in slack 😄

I was thinking I could work on documentation to help get people on board with using dep

YES! We really need this. Once we got the FAQ together we sorta dropped the documentation thing - #331 - but it's still quite important. Especially as we near this first bit of "stability."

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants