feat: add UUIDv7 support with encoding options#65
feat: add UUIDv7 support with encoding options#65ashwingopalsamy wants to merge 3 commits intolithammer:masterfrom
UUIDv7 support with encoding options#65Conversation
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
|
I think it's possible to reuse namespace logic like so: func NewWithNamespace(name string) string {
return newWithNamespace(name, uuid.NewRandom)
}
func NewV7WithNamespace(name string) string {
return NewWithNamespace(name, uuid.NewV7)
}
func newWithNamespace(name string, defaultUUID func() (uuid.UUID, error)) string {
var u uuid.UUID
switch {
case name == "":
u = uuid.Must(defaultUUID())
case hasPrefixCaseInsensitive(name, "https://"):
u = hashedUUID(uuid.NameSpaceURL, name)
case hasPrefixCaseInsensitive(name, "http://"):
u = hashedUUID(uuid.NameSpaceURL, name)
default:
u = hashedUUID(uuid.NameSpaceDNS, name)
}
return DefaultEncoder.Encode(u)
}Or maybe, it would be even better, to just introduce exported (naming is up for discussion) MustNewWithNamespace(name, uuid.NewV6)
MustNewWithNamespace(name, uuid.NewV7) |
|
Hey, thanks for reviewing. Its a good idea, but I feel that we should keep these functions separate. Here’s why: Separate functions like Also, I think this abstraction does not solve a real problem. The namespace logic is simple and unlikely to change often. Adding an exported function like Simplicity has its value here. Keeping the functions distinct makes the library clean and predictable; it does what users expect, with no extra layers to parse or understand. That said, I get the intention behind the suggestion. Let me know if there’s something that I'm overlooking. |
|
@lithammer - would it be possible for you to review this PR? |
| // NewV7WithNamespace returns a new UUIDv5 (or v7 if name is empty), encoded with base57. | ||
| func NewV7WithNamespace(name string) string { |
There was a problem hiding this comment.
I think the naming for this function becomes a bit confusing when it says NewV7 but typically you actually get a V5.
The NewWithNamespace function gets away with it because it's not explicitly mentioned. Hmm 🤔
There was a problem hiding this comment.
I don't have any great suggestions though.
There was a problem hiding this comment.
I'd say drop it for now. I think I want to move away from NewWithNamespace in favour of an explicit NewV5(ns, name) function, see:
The risk with trying to be smart is that people want to add new schemes to the name→namespace resolver, e.g:
Better to just help them create their own "namespace matcher" function. They can tailor it to their own use-case as well (for example don't bother with case-insensitive matching etc)
Changes
Addressing the issue #61, this PR introduces
UUIDv7support toshortuuid, bringing time-ordered UUID generation to our library.What does the PR implement?
NewV7: Generates a UUIDv7 using the default base57 alphabet.NewV7WithEncoder: Allows encoding UUIDv7 with a custom encoder.NewV7WithNamespace: Creates a UUIDv5 (or UUIDv7 if the namespace is empty) and encodes it.NewV7WithAlphabet: Encodes UUIDv7 with a custom alphabet.Adds Unit Tests including Benchmarks. No breaking changes; everything is backward-compatible.