search

gofrs / uuid

A UUID package for Go
MIT License
1.57k stars 110 forks source link

Add a New() method to wrap NewV4() with Must() #101

Closed suber124 closed 1 year ago

suber124 commented 2 years ago

The change is to add a New() method to just return a v4 UUID without error. It wraps the Must() method. Similar to this New() API.

peterstace commented 2 years ago

This seems fairly asymmetrical to me. I don't think there's anything special about V4 UUIDs such they should get special treatment. If anything, this would make more sense to me named as MustV4 (with corresponding methods added for other UUID versions as well).

suber124 commented 2 years ago

@bryanbaek 🥰

suber124 commented 2 years ago

This seems fairly asymmetrical to me. I don't think there's anything special about V4 UUIDs such they should get special treatment. If anything, this would make more sense to me named as MustV4 (with corresponding methods added for other UUID versions as well).

Hi @peterstace, thanks for the comment. 🥰 There is nothing special about the v4. Compared with the google/uuid New() API, I just found gofrs/uuid is too verbose to use. I believe most of the time, people do not care which version of the UUID is used plus how we want to handle the error.

fred2167 commented 2 years ago

I also found generating methods are too verbose and I don't usually care which version I am using. A more compact signature is much appreciated

cameracker commented 2 years ago

Thanks for the submission. Personally, I'm opposed to adding this to the API. The existing interface is still relatively compact, and it's fairly easy to add this in user land. Having the library provide a guided default to v4 is more opinionated than I think the library should be.

I understand the sentiment that most people aren't going to care what uuid format they're using to get their entropy, but there are cases where it does matter, and it probably pays to be a deliberate with what format you're using. The IDs are different, and are optimized for different cases (especially the new v6-v7 ones).

Additionally, this will mean that New() will forever mean v4 and it'll require a breaking change for us to change what the default is. That doesn't seem to me like a decision we want to take on later. The migration change seems better handled in userland where it's just the individual app being impacted, rather than us trying to grapple with the impact to N downstream users.

For the increase in API surface area (however small) I don't think the API or its users will benefit substantially.

codecov-commenter commented 2 years ago

Codecov Report

Merging #101 (471d4a4) into master (028e840) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #101   +/-   ##
=======================================
  Coverage   99.27%   99.27%           
=======================================
  Files           4        4           
  Lines         551      553    +2     
=======================================
+ Hits          547      549    +2     
  Misses          3        3           
  Partials        1        1
Impacted Files Coverage Δ
generator.go 98.67% <100.00%> (+<0.01%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cameracker commented 1 year ago

Hi all, I'm going to close this PR. Thanks very much for the submission. If there's renewed interest, we can reopen it.