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

JsonReader.readJsonValue: parse number as int/long/dbl #314

Closed
wants to merge 2 commits into from

Conversation

simon04
Copy link

@simon04 simon04 commented May 25, 2017

Attempt an exact conversion to integer or long prior before returning a double.

This addresses influxdata/influxdb-java#276.

@@ -297,7 +297,7 @@ JsonReader newReader(String json) throws IOException {
+ "2.718281828459045]";
JsonReader reader = newReader(json);
reader.beginArray();
assertThat(reader.nextDouble()).isEqualTo(-0.0);
assertThat(reader.nextDouble()).isIn(0.0, -0.0);
Copy link
Author

Choose a reason for hiding this comment

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

The distinction ±0.0 is lost in the JsonValueReader since BigDecimal does not distinguish between those.

Attempt an exact conversion to integer or long prior before returning a double.
@swankjesse
Copy link
Collaborator

Thanks for sending this. Unfortunately, I don’t think we want to change the default behavior here.

In particular I think these two JSONs should be equal when converted to Java.
[1,2,3], [1.0,2.0,3.0]

Unfortunately, a Java Long is not equal to the corresponding Double, so we need to either pick the data type based on the encoding, or pick one data type universally.

For Moshi’s defaults we’ve chosen to use a single data type, double, to use everywhere.

That said, it is possible to get this behavior with a JsonAdapter. Lemme try to supply that...

@simon04
Copy link
Author

simon04 commented May 26, 2017

Thanks for your answer. The JSON standard is pretty lose on the interpretation of numbers. So there's no clear correct or wrong.

I'm highly interested in the mentioned JsonAdapter.

@swankjesse
Copy link
Collaborator

@simon04 here’s a JSON adapter that does what we’re talking about. Unfortunately it didn’t work immediately as I was hoping so I submitted this PR. You’ll need a snapshot or the next release before this adapter works as you want.

https://gist.github.com/swankjesse/0331f24df1aa25454eaae04e49fe2f19

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.

2 participants