add Version4 factory to configure UUID generation

[rittneje]

Adds a Version4 factory struct, which can accumulate options for generating version 4 UUIDs. For now the only option is the io.Reader used for randomness.

Also reverts #80, as that implementation cannot be used safely in general. Instead, clients should create a factory with a buffered io.Reader implementation.

Fixes #86.

cc @pborman

[joewreschnig]

I have similar concerns about this that I have about the pool it's replacing - it's a big new API surface that doesn't really relate to "generating and inspecting" UUIDs. It's nothing that couldn't be done in a wrapper library or application, and doesn't abstract anything about UUIDs themselves.

This package should focus on efficient and correct UUID handling and making sure the UUID follows go best practices; it doesn't need to provide every sugared ctor.

[joewreschnig]

Also, maybe more objectively than some point about API taste, it doesn't solve half of the performance issue the pool did, that of requiring an allocation per call to New:

func BenchmarkUUID_NewVersion4(b *testing.B) {
    b.RunParallel(func(pb *testing.PB) {
        v4 := Version4{bufio.NewReaderSize(rander, 16*16)}
        for pb.Next() {
            _, err := v4.New()
            if err != nil {
                b.Fatal(err)
            }
        }
    })
}

BenchmarkUUID_NewVersion4-4      6922863           172.8 ns/op        16 B/op          1 allocs/op

[rittneje]

@joewreschnig The reason this approach is preferable to yours in #89 is because this will not lead to a combinatorial explosion in the number of constructors. It would be trivial to add some ReadInto(*UUID) error method that directly addresses your desire, without having to have two separate versions of it (one that takes the io.Reader and one that defaults to crypto/rand.Rand). And if more options pertaining to the UUID generation arise, they can easily be added to the Version4 struct, again without any combinatorial explosion.

The only reason I did not add said constructor was to not bloat this PR, since there is no existing functionality to have parity with.

[joewreschnig]

If the project had a long history of adding options I would agree, but the ctor interfaces have been stable for years. The best way to ensure a complex set of options is to make an object to stash them in, instead of figuring out the fewer relevant primitives. (That was also the mistake in the pool API.)

no existing functionality to have parity with.

This doesn't seem fair; the reason there's no zero-allocation ctor to claim parity with is because you took it out.

[rittneje]

This doesn't seem fair; the reason there's no zero-allocation ctor to claim parity with is because you took it out.

I don't understand what you are referring to. There is no constructor to generate into a pre-allocated UUID, hence your PR.

[rittneje]

If you want to add a new factory type for V4 it really does not need to be in this package as the user has no way to get to it without calling it explicitly.

I'm not sure what you mean by this. The user has no way to get to what?

A solution to the issue of not knowing what libraries are doing, the easy solution is to simply disallow the calling of EnableRandPool and DisableRandPool outside of package main.

How do you intend to enforce that?

[pborman]

If you want to add a new factory type for V4 it really does not need to be in this package as the user has no way to get to it without calling it explicitly.

I'm not sure what you mean by this. The user has no way to get to what?

The only way to use the new factory is to write new code. They have to have a new factor (e.g., uuid.Version4) and then call NewUUID on it. No existing code would ever use it. Existing libraries will not use it even if you want to use it. Because of this you can easily create a package that does exactly this and does not require changes to the UUID package.

A solution to the issue of not knowing what libraries are doing, the easy solution is to simply disallow the calling of EnableRandPool and DisableRandPool outside of package main.

How do you intend to enforce that?

Using the reflect and runtime packages it is easy to determine what package your caller is in. The functions would make the check and if they are not called from main then they panic.

[rittneje]

After discussing with my team, we have decided to use our own version of this library instead, as it seems you have a different vision for it than we do.