-
Notifications
You must be signed in to change notification settings - Fork 104
Support JSON 1.0 release #400
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
Conversation
| ft = typeof(field) | ||
| value = ft <: get(SUPPORTED_TYPES, nameof(ft), Union{}) ? JSON.lower(field) : field | ||
| d[name] = value | ||
| d[name] = value isa Float64 && !isfinite(value) ? nothing : value |
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.
This was a surprise to me; I didn't realize JSON pre-1.0 allowed writing Inf out by default (which is against JSON spec) as null. I tried to think about other ways we could try and compat this, but I think this is simplest.
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.
Python does
>>> import json, math
>>> json.dumps(math.inf)
'Infinity'
225e4e0 to
7dbf408
Compare
odow
left a comment
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.
👍 this is blocking JuMP from updating to JSON@1
|
Not sure who is best to merge; @willow-ahrens perhaps? |
|
I guess @vchuravy has commit permissions. |
|
This looks great, do you also need a new release? |
|
Yes |
|
Not sure what's going on with the queued job. But do we really need to test v1.7 on ubuntu 20.04? |
|
I haven't investigate far yet, but this new release appears to be a breaking change that causes BaseBenchmarks to now fail. We might need to yank this version? |
Would prefer that we fix this bug and released a new patch. Yanking would break stuff like this lanl-ansi/PowerModels.jl#986, where I'm updating to JSON@1, and which have BenchmarkTools in their dependency graph. |
|
Issue seems to be (json) pkg> st
Status `/private/tmp/json/Project.toml`
[682c06a0] JSON v1.1.0
julia> import JSON
julia> JSON.json(Dict(("a", "b") => 1))
ERROR: ArgumentError: No key representation for Tuple{String, String}. Define StructUtils.lowerkey(::Tuple{String, String})
Stacktrace:
[1] lowerkey(::JSON.JSONWriteStyle, x::Tuple{String, String})
@ JSON ~/.julia/packages/JSON/jLnej/src/write.jl:212
[2] applyeach
@ ~/.julia/packages/StructUtils/CvpPP/src/StructUtils.jl:563 [inlined]
[3] json!(buf::Vector{…}, pos::Int64, x::Dict{…}, opts::JSON.WriteOptions{…}, ancestor_stack::Vector{…}, io::Nothing, ind::Int64, depth::Int64, bufsize::Int64)
@ JSON ~/.julia/packages/JSON/jLnej/src/write.jl:617
[4] json! (repeats 3 times)
@ ~/.julia/packages/JSON/jLnej/src/write.jl:562 [inlined]
[5] json(x::Dict{Tuple{String, String}, Int64}; pretty::Bool, kw::@Kwargs{})
@ JSON ~/.julia/packages/JSON/jLnej/src/write.jl:433
[6] json(x::Dict{Tuple{String, String}, Int64})
@ JSON ~/.julia/packages/JSON/jLnej/src/write.jl:426
[7] top-level scope
@ REPL[5]:1
Some type information was truncated. Use `show(err)` to see complete types.Where previously julia> import JSON
julia> JSON.json(Dict(("a", "b") => 1))
"{\"(\\\"a\\\", \\\"b\\\")\":1}" |
|
Reproducer for this package is: julia> using BenchmarkTools
julia> grp = BenchmarkTools.BenchmarkGroup();
julia> grp["a", "b"] = BenchmarkTools.BenchmarkGroup()
0-element BenchmarkTools.BenchmarkGroup:
tags: []
julia> BenchmarkTools.save(stdout, grp)
ERROR: ArgumentError: No key representation for Tuple{String, String}. Define StructUtils.lowerkey(::Tuple{String, String}) |
|
One possible fix for BenchmarkTools is #406 |
|
A fix has been merged upstream in JSON to avoid breaking this pre-1.0 compat issue |
No description provided.