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.

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

vendor: Update json-iterator #6030

Merged
merged 1 commit into from Sep 17, 2019
Merged

vendor: Update json-iterator #6030

merged 1 commit into from Sep 17, 2019

Conversation

gouthamve
Copy link
Member

Also add explicit support for NaN/Inf

Signed-off-by: Goutham Veeramachaneni gouthamve@gmail.com

@gouthamve gouthamve changed the title vendor: Update json-iterator vendor: Update json-iterator and client_golang Sep 17, 2019
@gouthamve gouthamve force-pushed the update-json-iterator branch 2 times, most recently from c9bb021 to 50a34ce Compare September 17, 2019 06:25
@gouthamve gouthamve changed the title vendor: Update json-iterator and client_golang vendor: Update json-iterator Sep 17, 2019
stream.WriteFloat64(p.V)

// Taken from https://github.com/json-iterator/go/blob/master/stream_float.go#L71 as a workaround
// to https://github.com/json-it
Copy link
Member

Choose a reason for hiding this comment

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

Was this supposed to be a link to a GitHub issue? It looks truncated.

@juliusv
Copy link
Member

juliusv commented Sep 17, 2019

Could you elaborate on "Also add explicit support for NaN/Inf", how/where that's achieved?

@juliusv
Copy link
Member

juliusv commented Sep 17, 2019

(just wondering because the last time I tried to update this json-iter, it resulted in NaN/Inf/-Inf causing marshaling errors, so I downgraded it again)

@juliusv
Copy link
Member

juliusv commented Sep 17, 2019

Ah I guess that's just generally the manual strconv.AppendFloat(), taking that out of the hands of the now-buggy json-iter. But was there any other reason to update to the buggy json-iter version?

Also add explicit support for NaN/Inf

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
Copy link
Member

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

LGTM

@bboreham
Copy link
Member

From my point of view it's to match prometheus/client_golang#570

However the jsoniter change was deliberate so the other choice would be to fork it?

@brian-brazil
Copy link
Contributor

👍

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

LGTM if we have unit tests for that (:

@@ -1266,10 +1266,23 @@ func marshalPointJSON(ptr unsafe.Pointer, stream *jsoniter.Stream) {
}
stream.WriteMore()
stream.WriteRaw(`"`)
stream.WriteFloat64(p.V)

// Taken from https://github.com/json-iterator/go/blob/master/stream_float.go#L71 as a workaround
Copy link
Member

Choose a reason for hiding this comment

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

Is this tested properly in api_test.go?

Copy link
Contributor

Choose a reason for hiding this comment

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

I find your lack of faith... disturbing:

response: promql.Point{V: math.NaN(), T: 0},

Copy link
Member

Choose a reason for hiding this comment

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

😢 I am too lazy

@gouthamve gouthamve merged commit 4c648ed into master Sep 17, 2019
@gouthamve gouthamve deleted the update-json-iterator branch September 17, 2019 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants