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

Add support for uint and uint64 #116

Merged
merged 5 commits into from
Aug 2, 2016
Merged

Add support for uint and uint64 #116

merged 5 commits into from
Aug 2, 2016

Conversation

creiht
Copy link
Contributor

@creiht creiht commented Aug 1, 2016

Fixes #110

Before opening your pull request, please make sure that you've:

Thanks for your contribution!

@creiht
Copy link
Contributor Author

creiht commented Aug 1, 2016

I know you guys are in the middle of a refactor, but wanted to make sure I was on the right path. Let me know when you are done making internal changes, and I will refactor this fix accordingly. Thanks!

prashantv
prashantv reviewed Aug 1, 2016
View reviewed changes
json_encoder.go
// AddUInt adds a string key and integer value to the encoder's fields. The key
// is JSON-escaped.
func (enc *jsonEncoder) AddUInt(key string, val uint) {
enc.AddInt64(key, int64(val))
Copy link
Collaborator

Choose a reason for hiding this comment

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

we'll need to use strconv.AppendUint here, otherwise very large values will actually be printed as negative numbers (since the int64 will overflow).

Same below for AddUint64. You probably want to use strconv.AppendUint int AddUint64, and AddUint can just call AddUint64.

@akshayjshah We should consider simplifying the Encoder interface for int64 and uint64 to remove AddInt and AddUint and just have the 64-bit versions, and the field can cast itself.

@creiht
Fixed uint64 encoding to use strconv.AppendUint64
97af8fb 
Also fixed tests to use large ints that would overflow normal values
@creiht
Copy link
Contributor Author

creiht commented Aug 1, 2016

Thanks for the catch. Updated the pull requests accordingly.

@akshayjshah
Copy link
Contributor

Looks great! Only one nit: instead of UInt (with a capital I), can we match the standard library and use Uint (with a lower-case i)? Other than that, this looks good to me.

akshayjshah pushed a commit that referenced this pull request Aug 2, 2016
Add support for unsigned ints
b7bf509 
In advance of #116, add support for unsigned ints in the text encoder.
akshayjshah pushed a commit that referenced this pull request Aug 2, 2016
Add a TextEncoder for human-readable output
cb0d867 
When logging to console, JSON isn't the most friendly output - it's nice
for machines but not optimal for humans. This PR adds a text encoder
that prioritizes human readability, along with a few time-handling
options for that encoder.

In advance of #116, this PR also includes support for unsigned ints.
@akshayjshah akshayjshah mentioned this pull request Aug 2, 2016
Merged
akshayjshah pushed a commit that referenced this pull request Aug 2, 2016
Add a TextEncoder for human-readable output
7194726 
When logging to console, JSON isn't the most friendly output - it's nice
for machines but not optimal for humans. This PR adds a text encoder
that prioritizes human readability, along with a few time-handling
options for that encoder.

In advance of #116, this PR also includes support for unsigned ints.
prashantv
prashantv reviewed Aug 2, 2016
View reviewed changes
json_encoder_test.go
@@ -114,8 +115,12 @@ func TestJSONEncoderFields(t *testing.T) {
{"bool", `"k\\":true`, func(e Encoder) { e.AddBool(`k\`, true) }},
{"int", `"k":42`, func(e Encoder) { e.AddInt("k", 42) }},
{"int", `"k\\":42`, func(e Encoder) { e.AddInt(`k\`, 42) }},
{"int64", `"k":42`, func(e Encoder) { e.AddInt64("k", 42) }},
{"int64", `"k\\":42`, func(e Encoder) { e.AddInt64(`k\`, 42) }},
{"int64", fmt.Sprintf(`"k":%d`, math.MaxInt64), func(e Encoder) { e.AddInt64("k", math.MaxInt64) }},
Copy link
Collaborator

@prashantv prashantv Aug 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for updating these tests, can you also add MinInt64 to the int64 test?

Just a single test for that (don't need to duplicate the key escaping test)

@creiht
Copy link
Contributor Author

creiht commented Aug 2, 2016

I've updated the naming and added a test for MinInt64. Let me know if you see anything else.

@akshayjshah
Copy link
Contributor

Hi Chuck! Looks good to me, but I don't see you in our list of CLA signatories. Have you signed the CLA? Note that you'll have to use the correct GitHub username (creiht) for it to count.

@creiht
Copy link
Contributor Author

creiht commented Aug 2, 2016

Ahh sorry, the browser auto-completed the github username and didn't realize it. Should be updated now.

@akshayjshah
Copy link
Contributor

Cool - thanks for the contribution (and your patience)! Merging this now.

@akshayjshah akshayjshah merged commit b56bc62 into uber-go:master Aug 2, 2016
@blampe
Copy link

blampe commented Aug 2, 2016

Thank you for the PR, Chuck!

ascandella pushed a commit that referenced this pull request Sep 16, 2016
Add a TextEncoder for human-readable output
b794f33 
When logging to console, JSON isn't the most friendly output - it's nice
for machines but not optimal for humans. This PR adds a text encoder
that prioritizes human readability, along with a few time-handling
options for that encoder.

In advance of #116, this PR also includes support for unsigned ints.
ascandella pushed a commit that referenced this pull request Sep 25, 2016
Add a TextEncoder for human-readable output
38c3add 
When logging to console, JSON isn't the most friendly output - it's nice
for machines but not optimal for humans. This PR adds a text encoder
that prioritizes human readability, along with a few time-handling
options for that encoder.

In advance of #116, this PR also includes support for unsigned ints.
ascandella pushed a commit that referenced this pull request Sep 25, 2016
Add a TextEncoder for human-readable output
00a304c 
When logging to console, JSON isn't the most friendly output - it's nice
for machines but not optimal for humans. This PR adds a text encoder
that prioritizes human readability, along with a few time-handling
options for that encoder.

In advance of #116, this PR also includes support for unsigned ints.
ascandella pushed a commit that referenced this pull request Sep 25, 2016
Add a TextEncoder for human-readable output
10a0c8f 
When logging to console, JSON isn't the most friendly output - it's nice
for machines but not optimal for humans. This PR adds a text encoder
that prioritizes human readability, along with a few time-handling
options for that encoder.

In advance of #116, this PR also includes support for unsigned ints.
ascandella pushed a commit that referenced this pull request Sep 25, 2016
Add a TextEncoder for human-readable output
f979bf7 
When logging to console, JSON isn't the most friendly output - it's nice
for machines but not optimal for humans. This PR adds a text encoder
that prioritizes human readability, along with a few time-handling
options for that encoder.

In advance of #116, this PR also includes support for unsigned ints.
ascandella pushed a commit that referenced this pull request Sep 25, 2016
Add a TextEncoder for human-readable output
0bbb077 
When logging to console, JSON isn't the most friendly output - it's nice
for machines but not optimal for humans. This PR adds a text encoder
that prioritizes human readability, along with a few time-handling
options for that encoder.

In advance of #116, this PR also includes support for unsigned ints.
ascandella pushed a commit that referenced this pull request Sep 26, 2016
@akshayjshah
Add a human-optimized text encoder (#123)
a26b259 
* Add a TextEncoder for human-readable output

When logging to console, JSON isn't the most friendly output - it's nice
for machines but not optimal for humans. This PR adds a text encoder
that prioritizes human readability, along with a few time-handling
options for that encoder.

In advance of #116, this PR also includes support for unsigned ints.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@creiht @akshayjshah @blampe @prashantv