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

RFC: Add allownan keyword argument to parse() (Fixes #168) #280

Merged
merged 3 commits into from
May 18, 2019

Conversation

kmsquire
Copy link
Contributor

  • Allows reading out-of-spec JSON files which include Nan, Infinity, and -Infinity

The ideal solution to reading out-of-spec JSON files was proposed by @TotalVerb in #169.

Until that is implemented, this provides a pragmatic solution for people to read these types of JSON files when they don't have control over the file generation.

@kmsquire kmsquire changed the title RFC: Add allownan keyword argument to parse() RFC/WIP: Add allownan keyword argument to parse() (Fixes #168) May 13, 2019
@kmsquire
Copy link
Contributor Author

There are no tests yet. I'll wait for any discussion on whether or not this is desired first.

@kmsquire kmsquire requested review from TotalVerb and ararslan May 13, 2019 18:32
@ararslan
Copy link
Member

ararslan commented May 13, 2019

Seems okay to me in principle but I'm not all that familiar with the JSON spec nor the internals of this package so I'm probably not the best person to review this.

@TotalVerb
Copy link
Collaborator

I think that if we have a permissive mode like this we should also support the Python output nan, inf, -inf (note case). I'm not sure if any Python code generates this output though.

@kmsquire
Copy link
Contributor Author

I think that if we have a permissive mode like this we should also support the Python output nan, inf, -inf (note case). I'm not sure if any Python code generates this output though.

According to the Python json package docs, it outputs the JavaScript values:

If allow_nan is false (default: True), then it will be a ValueError to serialize out of range float values (nan, inf, -inf) in strict compliance of the JSON specification. If allow_nan is true, their JavaScript equivalents (NaN, Infinity, -Infinity) will be used.

simplejson and rapidjson follow the same convention. ujson doesn't output any of these values.

Google's JavaScript GSON module also accepts NaN, Infinity, -Infinity. It does not output them by default, but allows this behavior to be overridden (see
https://static.javadoc.io/com.google.code.gson/gson/2.6/com/google/gson/GsonBuilder.html#serializeSpecialFloatingPointValues()).

Given this, I don't think we need to support standard Python values in JSON.

@kmsquire
Copy link
Contributor Author

While looking through some of the packages in the previous comment, I ran across this snippet (from https://tools.ietf.org/html/rfc7159.html#section-9):

   A JSON parser transforms a JSON text into another representation.  A
   JSON parser MUST accept all texts that conform to the JSON grammar.
   A JSON parser MAY accept non-JSON forms or extensions.

Given this, I think what we're doing here is not too unreasonable.

That said

  • I still support @TotalVerb's original idea to make this a configurable. I just don't have time to work on that.
  • An alternative would be to fork this package and create a JSON5 package (see https://json5.org/), which accepts NaN, Infinity, and a number of other JSON extensions (comments, trailing commas, keys without quotes, single quoted strings, etc.). Again, takes time. I don't think json5 is widely used yet.

@kmsquire
Copy link
Contributor Author

I'll add tests, and see if anyone else wants to review.

Copy link
Collaborator

@TotalVerb TotalVerb left a comment

Choose a reason for hiding this comment

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

LGTM pending tests.

@kmsquire kmsquire changed the title RFC/WIP: Add allownan keyword argument to parse() (Fixes #168) RFC: Add allownan keyword argument to parse() (Fixes #168) May 16, 2019
@kmsquire
Copy link
Contributor Author

  1. I added tests
  2. I refactored the code to reduce code duplication. @TotalVerb, can you review again if you get the chance?

The codecov results are a bit hard to understand--it looks like we're not providing the right line numbers, and/or some of the code that was added was simply inlined later.

src/specialized.jl Outdated Show resolved Hide resolved
test/parser/nan-inf.jl Outdated Show resolved Hide resolved
src/specialized.jl Show resolved Hide resolved
src/Parser.jl Show resolved Hide resolved
src/Parser.jl Outdated Show resolved Hide resolved
@ararslan ararslan removed their request for review May 17, 2019 20:48
@kmsquire kmsquire requested a review from TotalVerb May 17, 2019 22:08
src/Parser.jl Show resolved Hide resolved
Copy link
Collaborator

@TotalVerb TotalVerb left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@kmsquire
Copy link
Contributor Author

kmsquire commented May 17, 2019

Okay, thanks @TotalVerb. Merging! (Edit: after squashing and running CI to make sure I didn't screw up the squash...)

@kmsquire kmsquire merged commit 9581e13 into master May 18, 2019
@kmsquire kmsquire deleted the kms/allownan branch May 18, 2019 00:22
Zentrik added a commit to Zentrik/JSON.jl that referenced this pull request Jan 4, 2024
Unfortunately this was never updated in JuliaIO#280 to be the correct default, and so has been wrong ever since introduction.
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.

4 participants