Implement fmt.Formatter

[dylan-bourque]

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]

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%> (ø)	⬆️

---

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]

I'm not convinced about this approach.

When UUID does not implement fmt.Formatter, this program fails the go vet check, and produces obviously wrong output: https://play.golang.org/p/stts25e3_2a

However, when UUID does implement fmt.Formatter, the program passes go vet, and produces more plausible looking (but wrong) output: https://play.golang.org/p/hNOR123ZjkU

The behavior in the second case seems dangerous to me. Using the wrong formatting verb is a programmer error and this implementation of fmt.Formatter hides it from go vet, and swallows it when it produces output. I don't know what a good solution to this problem might be, short of trying to re-create the original format string from the fmt.State and calling fmt.Sprintf with a [16]byte value instead of a UUID, to avoid recursion.

For reference, pkg/errors implements fmt.Formatter on some of its types, and when it doesn't understand a verb, it simply produces no output, e.g. https://github.com/pkg/errors/blob/master/stack.go#L64-L84. I'm not a huge fan of this approach either, but it is at least a data point we have, if we're going to implement fmt.Formatter at all.

[dylan-bourque]

There doesn't seem to be any way to indicate that a formatting verb is invalid/not supported, so it feels like "do nothing" is the only option. I'm okay with that path for all of the formatting verbs that don't apply: %b, %c, %d, %o, %U, %e, %E, %f, %F, %g, %G.

[acln0]

To clarify, "do nothing" means "emit no output"?

[dylan-bourque]

Correct, "do nothing" would be to return an empty string for all of those verbs.

[acln0]

Okay. I guess that's reasonable, and there is a precedent for it too. But my concern with respect to the fact that this conceals programmer errors such as using the wrong flags remains.

The fmt.Formatter implementation is opaque to static analysis tools like go vet. We're gaining convenience and a nice API for printing UUIDs as hexadecimal strings, but we are losing precision under the lens of static analysis tools. We should decide if this trade-off is worth making.

[acln0]

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]

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

[dylan-bourque]

@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]

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]

hex.EncodeToString(u.Bytes())

[dylan-bourque]

@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]

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.

[theckman]

Thank you for opening this PR, I think it's good feature for us to consider.

As part of #71 we're working towards removing an implementation within this package that falls outside of the RFC that standardized UUIDs, bringing us fully in alignment with the standard. This change would introduce functionality that makes us support non-standard compliant functionality[1].

I'm on the fence about supporting non-standard functionality, when it doesn't seem like a common need. Speaking from personal experience, I don't believe this format is widely used because it's against the RFC; and if that's not true please let me know.

Now looking at the RFC link above, it does show that a capitalized hexadecimal format is also supported and so I think the concept behind this change still solves a need. We could add the ability for consumers to render their UUIDs with capital letters instead of only lowercase letters.

For this PR as it is today, the question I think we need to answer is whether we want to take on the cost of maintaining code for those who aren't standards compliant? Secondarily, do we also want to support capitalized hexadecimal UUID formats? @gofrs/uuid

[1] https://tools.ietf.org/html/rfc4122#section-3

[dylan-bourque]

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]

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

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

[dylan-bourque]

I'll try to make these changes some time today

[acln0]

LGTM!

[martin308]

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

[theckman]

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

[theckman]

@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]

Amazing, thank you! 🙏