-
Notifications
You must be signed in to change notification settings - Fork 27
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.
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
friendlier formatting of top level byte slices #59
friendlier formatting of top level byte slices #59
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great with some suggestions, thanks!
format.go
Outdated
@@ -35,10 +36,28 @@ func Format(v interface{}) string { | |||
case string: | |||
return quoteString(v) | |||
} | |||
if v != nil && isByteSlice(v) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nil check is already present in isByteSlice, so is it redundant here (or there)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, good point. i adjusted the code a bit.
}, { | ||
about: "bytes", | ||
value: []byte("hello"), | ||
want: `[]uint8("hello")`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could s/uint8/byte/ given that the byte alias is absolutely more common.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed in principle, but it would be an intrusive change. Printf("%T")
prints []uint8
and the pretty package also prints []uint8
, so it's fairly consistent at least currently. I don't think it's worth doing now. If/when we replace kr/pretty
would be a good time to do it, I think.
format_test.go
Outdated
@@ -9,6 +9,8 @@ import ( | |||
qt "github.com/frankban/quicktest" | |||
) | |||
|
|||
type myBytes []byte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move this close to nilStringer after the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
}, { | ||
about: "bytes with invalid utf-8", | ||
value: []byte("\xff"), | ||
want: "[]uint8{0xff}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test with a []byte(nil)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. added the test and fixed the code.
got: | ||
[]uint8("null") | ||
want: | ||
json.RawMessage("null") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, isn't it? I only realised that you could do that this morning, when someone was asking for the equivalent of https://godoc.org/github.com/stretchr/testify/assert#JSONEq, which tests against a string.
d96e468
to
9e2c77f
Compare
As a specific use case, this makes the errors printed in verbose mode nicer when using `JSONEquals` with a `RawMessage` argument, but also should be a general improvement. It would be nice if it happened at deeper levels too, but this is a start.
9e2c77f
to
c17b1d9
Compare
As a specific use case, this makes the errors printed in verbose
mode nicer when using
JSONEquals
with aRawMessage
argument,but also should be a general improvement. It would
be nice if it happened at deeper levels too, but this is a start.