Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement fmt.Formatter #72

Merged
merged 8 commits into from Mar 20, 2019
Merged

Implement fmt.Formatter #72

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
61107f3
implement fmt.Formatter
dylan-bourque Feb 15, 2019
8817e46
use 'x'/'X' instead of 'h'/'H' for outputting only hex digits
dylan-bourque Feb 18, 2019
0af1998
removed commented out test cases from testUUIDFormat()
dylan-bourque Feb 18, 2019
8b125a7
refined behavior of UUID.Format() and updated unit tests to match
dylan-bourque Mar 14, 2019
094e8c2
removed old comment block from UUID.Format()
dylan-bourque Mar 14, 2019
5d8c397
Merge branch 'master' into implement-fmt-formatter
dylan-bourque Mar 14, 2019
90065f9
change to backtick-string (and removed escaped double-quotes)
dylan-bourque Mar 15, 2019
5f199e6
Merge branch 'implement-fmt-formatter' of https://github.com/dylan-bo…
dylan-bourque Mar 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 43 additions & 0 deletions uuid.go
View file
Open in desktop
Expand Up @@ -33,6 +33,8 @@ import (
"encoding/binary"
"encoding/hex"
"fmt"
"io"
"strings"
"time"
)

Expand Down Expand Up @@ -156,6 +158,47 @@ func (u UUID) String() string {
return string(buf)
}

// Format implements fmt.Formatter for UUID values. It accepts 'h' or 'H' as a format to output only
// the hex digits of the UUID. Any other format specifier returns the canonical RFC-4122 representation.
func (u UUID) Format(f fmt.State, c rune) {
//
// TODO: dylan.bourque - 2019-02-15
// . should we implement %v? if so, what is "a Go-syntax representation" of a UUID when using %#v?
//
switch c {
case 'h', 'H':
acln0 marked this conversation as resolved.
Show resolved Hide resolved
s := hex.EncodeToString(u.Bytes())
if c == 'H' {
s = strings.Map(toCapitalHexDigits, s)
}
_, _ = io.WriteString(f, s)
case 'q':
acln0 marked this conversation as resolved.
Show resolved Hide resolved
_, _ = io.WriteString(f, `"`+u.String()+`"`)
default:
_, _ = io.WriteString(f, u.String())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

func toCapitalHexDigits(ch rune) rune {
// convert a-f hex digits to A-F
switch ch {
case 'a':
return 'A'
case 'b':
return 'B'
case 'c':
return 'C'
case 'd':
return 'D'
case 'e':
return 'E'
case 'f':
return 'F'
default:
return ch
}
}

// SetVersion sets the version bits.
func (u *UUID) SetVersion(v byte) {
u[6] = (u[6] & 0x0f) | (v << 4)
Expand Down
25 changes: 25 additions & 0 deletions uuid_test.go
View file
Open in desktop
Expand Up @@ -35,6 +35,7 @@ func TestUUID(t *testing.T) {
t.Run("Variant", testUUIDVariant)
t.Run("SetVersion", testUUIDSetVersion)
t.Run("SetVariant", testUUIDSetVariant)
t.Run("Format", testUUIDFormat)
}

func testUUIDBytes(t *testing.T) {
Expand Down Expand Up @@ -114,6 +115,30 @@ func testUUIDSetVariant(t *testing.T) {
}
}

func testUUIDFormat(t *testing.T) {
val := Must(FromString("12345678-90ab-cdef-1234-567890abcdef"))
tests := []struct {
u UUID
f string
want string
}{
{u: val, f: "%s", want: "12345678-90ab-cdef-1234-567890abcdef"},
{u: val, f: "%q", want: `"12345678-90ab-cdef-1234-567890abcdef"`},
{u: val, f: "%h", want: "1234567890abcdef1234567890abcdef"},
{u: val, f: "%H", want: "1234567890ABCDEF1234567890ABCDEF"},
{u: val, f: "%v", want: "12345678-90ab-cdef-1234-567890abcdef"},
{u: val, f: "%+v", want: "12345678-90ab-cdef-1234-567890abcdef"},
{u: val, f: "%#v", want: "12345678-90ab-cdef-1234-567890abcdef"},
{u: val, f: "%T", want: "uuid.UUID"},
}
for _, tt := range tests {
got := fmt.Sprintf(tt.f, tt.u)
if tt.want != got {
t.Errorf("Format(\"%s\") got %s, want %s", tt.f, got, tt.want)
dylan-bourque marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

func TestMust(t *testing.T) {
sentinel := fmt.Errorf("uuid: sentinel error")
defer func() {
Expand Down