Creates a virtual package at v3 subdirectory. #66

itsjamie · 2018-12-04T00:45:39Z

Step 1 in removing go modules opt-in support without breaking any
builds.

Step 1 in removing go modules opt-in support without breaking any builds.

itsjamie · 2018-12-04T00:47:24Z

Copied forward.go implementation to package.

I renamed the package identifier to match the folder name... but I'm realizing this breaks the whole idea, so changing to be uuid and deal with the duplicate name.

itsjamie · 2018-12-04T00:53:16Z

package main

import (
	"github.com/davecgh/go-spew/spew"
	"github.com/gofrs/uuid/v3"
)

func main() {
	u, _ := uuid.NewV4()
	spew.Dump(u)
}

works with GO111MODULE=off go run

itsjamie · 2018-12-04T01:57:12Z

See #65 .

dylan-bourque · 2018-12-04T14:45:21Z

I tested this change against a trivial "client" (import the uuid package and print the result of uuid.NewV4() to stdout) in Go 1.8.7, 1.9.7, 1.10.5, and 1.11.1. For each version, I ran go build and go run main.go for each combination of: with/without the /v3 in the import path, with/without go.mod. I also installed Dep and ran dep init to check backwards compatibility w/ vendoring. Additionally, I created a "client" outside of $GOPATH under Go 1.11 with a go.mod.

All cases were good except importing github.com/gofrs/uuid/v3 under Go 1.8, but this is expected because type aliases are Go 1.9+.

acln0

Thanks @dylan-bourque. We should update CI to reflect this change, because the builds are failing at the moment. To begin with, we should drop < 1.9 due to type aliases. Secondly, we need to fix the build for 1.9, which is failing because -coverprofile was only made to accept multiple packages in 1.10. We only need test coverage for the root package anyway, since the /v3 forwarding package has no tests.

Also, the question remains on whether we should set GO111MODULE at all in Travis. Perhaps we should not. It probably doesn't make a difference anyway.

Aside from the little documentation change I suggested, I think things look good.

acln0 · 2018-12-05T01:21:08Z

v3/forward.go

@@ -19,7 +19,7 @@
 // OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
 // WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

-package v3
+package uuid


I think we should leave a note here, for godoc purposes. The note should say that this uuid package consists of forwarding declarations only, and that callers should look at the root godoc (godoc.org/github.com/gofrs/uuid) instead.

acln0 · 2018-12-05T01:28:53Z

One more note. It would have been awesome if we could have copied the tests over to the forwarding package and watched them all pass! But as it stands, the tests rely on quite a few internal details, and this PR is not the place for such a change. That is a matter for another day.

As we are using aliases to support the creation of a forwarding package to de-virtualize the module import path this limits support of the code to Go 1.9 consumers if they import the non-virtual path.

@acln0

Coverprofile only supports multi-package in Go 1.10+ and therefore this breaks the CI build when we added the forwarding package. See #66 comment from @acln0. As there are no tests in the alias package that surfaced this issue, we aren't removing any coverage. But it is something to be aware of if another package is added to this repo.

codecov-io · 2018-12-13T14:34:50Z

Codecov Report

Merging #66 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #66   +/-   ##
=======================================
  Coverage   98.92%   98.92%           
=======================================
  Files           4        4           
  Lines         279      279           
=======================================
  Hits          276      276           
  Misses          2        2           
  Partials        1        1

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 9f278e1...3db6bec. Read the comment docs.

acln0 · 2018-12-13T14:48:56Z

Thanks @itsjamie! I think this set of patches is ready now. Usually, I would squash such a set down to a single commit, to end up with a nicer git history, but it's up to you if you want to do it or not.

After this is merged, someone should cut and publish v3.2.0. Thanks.

thepudds · 2018-12-13T15:22:22Z

@itsjamie @acln0 sorry if this is off base (including I might have lost the thread here a bit on this general topic given there have been more than a few comments)... but I had thought the v3/go.mod here was going to end up with a require directive for github.com/gofrs/uuid to help enforce a user importing github.com/gofrs/uuid/v3 ends up with the latest github.com/gofrs/uuid?

See for example this comment #61 (comment) from Bryan Mills:

The forwarding itself should work fine, but if you're doing active development in a part of the tree that lacks a go.mod file, you'll need to periodically bump the requirements in the /v3 module to pick up the changes. (Probably every time you push a new tag to the +incompatible path you'll want to add that same tag — and the corresponding require directive — to the v3/go.mod file.)

(CC @bcmills or @myitcv for any additional comment)

myitcv · 2018-12-13T15:41:31Z

.travis.yml

@@ -16,7 +14,7 @@ env:
 before_install:
  - go get golang.org/x/tools/cmd/cover
 script:
-  - go test ./... -race -coverprofile=coverage.txt -covermode=atomic
+  - go test . -race -coverprofile=coverage.txt -covermode=atomic


Just a thought, but is it worth expanding the tests to include an explicit test of the behaviour you're expecting a user to see, with respect to import paths? That suite of tests might also include your expectations on GOPATH vs module mode in Go 1.11.

thepudds · 2018-12-13T16:38:14Z

I think part of the earlier intent of this change was also to encourage people to use github.com/gofrs/uuid over github.com/gofrs/uuid/v3?

For example, slightly older snippet from @acln0:

We ask them to remove /v3 from their import paths eventually, and switch to gofrs/uuid instead. gofrs/uuid remains +incompatible with modules, and all future development happens in the root of the directory. After a while, we phase /v3 away ...

I don't personally know if that is indeed is still the intent, but if it is the intent, would it make sense to add a quick note to the README to explain something like that?

For example, maybe something relatively brief, such as:

Note: the recommended import path for all non-module and module-based consumers is import "github.com/gofrs/uuid". (For legacy reasons, github.com/gofrs/uuid/v3 is also currently supported, but any consumer using that /v3 import path is encouraged to migrate to importing github.com/gofrs/uuid).

Or maybe that is no longer the intent?

acln0 · 2018-12-13T17:25:01Z

Thanks @thepudds and @myitcv for the pertinent comments. Maybe this isn't quite ready yet. Holding off until we resolve them. I'll find some time to work on this very soon.

dismissing in view of recent comments about documentation and testing

theckman

Would you please be able to squash these commits in to a single one that provides context behind the change in a digestable way?

The truth is I've been a bit detatched from this, and why we're needing to do this to support modules, and so that information would be helpful to me and others. I'm personally a bit annoyed with the idea of having separate folders with code to support package versions, but it's possible we made mistakes earlier that have prompted that.

It'd be good to get this on the radar of those who are championing the usage of modules, because to me it feels like it's been quite painful for us to make this move for such a simple project. Definitely in comparison to dep, glide, and a myriad of others.

Part of this need for me is because we're about to take over logrus, and they have added modules support without a major release. Is it because they omitted /vN from their go.mod file? I want to understand what we did wrong, to avoid making the same mistake in the future.

/cc @thepudds @myitcv

dylan-bourque · 2019-01-06T03:36:56Z

The "break" came from adding `go.mod` then making further changes on the same major version. That led to inconsistent imports when using ` github.com/gofrs/uuid` vs `github.com/gofrs/uuid/v3`. IMO, the big miss on the part of the Go team was not loudly calling out that you need to do a major version bump when opting into modules if your package is already v2+. And I'm not really a fan of creating subfolders for each version but it's the only way for module-unaware importers to get semver behavior. I could get behind doing it for v3 for compatibility during this transition, but not really going forward. Re: logrus, if we're taking it over we'll more than likely want to do this same dance there.

…

On Sat, Jan 5, 2019, 9:17 PM Tim Heckman ***@***.***> wrote: ***@***.**** requested changes on this pull request. Would you please be able to squash these commits in to a single one that provides context behind the change in a digestable way? The truth is I've been a bit detatched from this, and why we're needing to do this to support modules, and so that information would be helpful to me and others. I'm personally a bit annoyed with the idea of having separate folders with code to support package versions. It'd be good to get this on the radar of those who are championing the usage of modules, because to me it feels like it's been quite painful. Part of this need for me is because we're about to take over logrus, and they have added modules support without a major release. Is it because they omitted /vN from their go.mod file? I want to understand what we did wrong, to avoid making the same mistake in the future. /cc @thepudds <https://github.com/thepudds> @myitcv <https://github.com/myitcv> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#66 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQ9re5nc_RNglpIRXxWlNvJxNA5FabKWks5vAWrMgaJpZM4Y_skU> .

itsjamie · 2019-01-07T17:11:54Z

After conversation in the #gofrs Slack, I'm going to close this pull-request in favour of removal of the go.mod file.

itsjamie · 2019-01-10T14:07:32Z

I was confused.

Fix #61. Creates a virtual package at v3 subdirectory.

22ea5ac

Step 1 in removing go modules opt-in support without breaking any builds.

itsjamie requested a review from acln0 December 4, 2018 00:45

Change exposed package name of real v3 path to uuid.

862d8fe

Create go.mod in v3 subpackage.

0363640

acln0 requested changes Dec 5, 2018

View reviewed changes

acln0 mentioned this pull request Dec 5, 2018

unify import paths under /v4 #62

Closed

stanislas-m mentioned this pull request Dec 8, 2018

Migrate to gofrs/uuid from gobuffalo/uuid gobuffalo/pop#188

Closed

itsjamie added 2 commits December 13, 2018 10:27

Remove coverage of Go 1.7 and Go 1.8

17ae1e2

As we are using aliases to support the creation of a forwarding package to de-virtualize the module import path this limits support of the code to Go 1.9 consumers if they import the non-virtual path.

acln0 previously approved these changes Dec 13, 2018

View reviewed changes

myitcv reviewed Dec 13, 2018

View reviewed changes

theckman requested changes Jan 6, 2019

View reviewed changes

itsjamie closed this Jan 7, 2019

itsjamie reopened this Jan 9, 2019

itsjamie mentioned this pull request Jan 9, 2019

Remove support for Go 1.11 modules #67

Merged

itsjamie closed this Jan 10, 2019

itsjamie deleted the fix-61 branch March 25, 2020 02:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creates a virtual package at v3 subdirectory. #66

Creates a virtual package at v3 subdirectory. #66

itsjamie commented Dec 4, 2018

itsjamie commented Dec 4, 2018

itsjamie commented Dec 4, 2018

itsjamie commented Dec 4, 2018

dylan-bourque commented Dec 4, 2018

acln0 left a comment

acln0 Dec 5, 2018

acln0 commented Dec 5, 2018 •

edited

Loading

codecov-io commented Dec 13, 2018 •

edited

Loading

acln0 commented Dec 13, 2018 •

edited

Loading

thepudds commented Dec 13, 2018 •

edited

Loading

myitcv Dec 13, 2018

thepudds commented Dec 13, 2018

acln0 commented Dec 13, 2018

theckman left a comment •

edited

Loading

dylan-bourque commented Jan 6, 2019 via email

itsjamie commented Jan 7, 2019

itsjamie commented Jan 10, 2019

Creates a virtual package at v3 subdirectory. #66

Creates a virtual package at v3 subdirectory. #66

Conversation

itsjamie commented Dec 4, 2018

itsjamie commented Dec 4, 2018

itsjamie commented Dec 4, 2018

itsjamie commented Dec 4, 2018

dylan-bourque commented Dec 4, 2018

acln0 left a comment

Choose a reason for hiding this comment

acln0 Dec 5, 2018

Choose a reason for hiding this comment

acln0 commented Dec 5, 2018 • edited Loading

codecov-io commented Dec 13, 2018 • edited Loading

Codecov Report

acln0 commented Dec 13, 2018 • edited Loading

thepudds commented Dec 13, 2018 • edited Loading

myitcv Dec 13, 2018

Choose a reason for hiding this comment

thepudds commented Dec 13, 2018

acln0 commented Dec 13, 2018

theckman left a comment • edited Loading

Choose a reason for hiding this comment

dylan-bourque commented Jan 6, 2019 via email

itsjamie commented Jan 7, 2019

itsjamie commented Jan 10, 2019

acln0 commented Dec 5, 2018 •

edited

Loading

codecov-io commented Dec 13, 2018 •

edited

Loading

acln0 commented Dec 13, 2018 •

edited

Loading

thepudds commented Dec 13, 2018 •

edited

Loading

theckman left a comment •

edited

Loading