dylan-bourque commented 5 years ago

I had a use case where I needed to get the string representation of a UUID with only the hex digits. Implementing fmt.Formatter seems to be the prescribed way to provide custom string formatting in Go, so I did so. Additionally, UUID.String() returns the RFC-compliant string so I did not touch that code.

The standard %s, %q and %v format runes have the same behavior as before. I added %h and %H for outputting only the hex digits without the dash (-) separators. %h uses 'a'-'f' and %H uses 'A-F'

codecov-io commented 5 years ago

Codecov Report

Merging #72 into master will increase coverage by 0.13%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
+ Coverage   98.92%   99.05%   +0.13%     
==========================================
  Files           4        4              
  Lines         279      318      +39     
==========================================
+ Hits          276      315      +39     
  Misses          2        2              
  Partials        1        1

Impacted Files	Coverage Δ
uuid.go	`100% <100%> (ø)`	:arrow_up:

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 e684523...5f199e6. Read the comment docs.

acln0 commented 5 years ago

To clarify: I'm not sure we should be doing this, but if we decide to, then I think it's not ready yet, and we need some good answers, especially to the third comment I left above.

cratermoon commented 5 years ago

What's wrong with the caller using strings.Replace()? https://play.golang.org/p/fPJoSEQjZ1t

dylan-bourque commented 5 years ago

@cratermoon Technically, nothing. Your example generates the desired result. But it loses context (because returning the hex digits of a UUID is not what strings.Replace() does). The idea was to provide a higher level logical abstraction. It also results in several unnecessary allocations while building the string with the dashes only to immediately remove them.

@acln0 I'll go through your suggestions tomorrow. Thanks for the thorough feedback.

dylan-bourque commented 5 years ago

As an alternative to implementing fmt.Formatter and all of the vagueness that seems to be involved with doing so, we could also implement .Format() directly like time.Time does, either as a package-level free function or as a method on the UUID type.

inliquid commented 5 years ago

hex.EncodeToString(u.Bytes())

dylan-bourque commented 5 years ago

@inliquid

The idea was to provide a higher level logical abstraction.

For reference, I'm personally not a fan of the "a little duplication is better than ..." mantra. To me, this is a logical operation on the UUID type and should be part of the package, not deferred to each consumer to re-implement.

dylan-bourque commented 5 years ago

I updated .Format() to use 'x' and 'X' instead of 'h' and 'H' for only the hex digits and updated the logic to return an empty string for unsupported format verbs.

dylan-bourque commented 5 years ago

The current implementation of UnmarshalText() is already coded to allow for the "hex digits, no separators, lowercase letters" format this PR proposes to add with the %x formatting verb, so a discussion about limiting the API strictly to what's defined in the RFC would need to consider that code also.

I don't know that we should limit the functionality to only what's defined in the RFC. Personally, I lend more weight to whether or not the functionality is logical and useful without breaking compliance with the underlying RFC specification. I think this PR adheres to all three of those conditions.

As far as usefulness, I've had a need for encoding/decoding UUID values in this format on two separate occasions now. I submitted the PR based on a guess that others may need/want this same behavior also, without punting to "You can do it yourself if you just ....".

I have no issue with the library implementing a superset of the RFC but I am far from the only one with an investment and/or opinion here.

theckman commented 5 years ago

This looks good. Wondering if we can add two other features to this to make it :100:.

I'm thinking I could hold the PR removing V2 support, and get this in to a 3.x release.

dylan-bourque commented 5 years ago

I'll try to make these changes some time today

martin308 commented 4 years ago

@theckman any plans to cut a new release with this change in?

theckman commented 4 years ago

@martin308 3.3.0 will be out within the next 30 minutes.

theckman commented 4 years ago

@martin308 https://github.com/gofrs/uuid/releases/tag/v3.3.0

Sorry for the delay, a few of us could've sworn we released it. Thanks for the ping!

martin308 commented 4 years ago

Amazing, thank you! 🙏

gofrs / uuid

Implement fmt.Formatter #72

Codecov Report