RFC: Introduce StructuralContext and Serialization for finer-grained serialization control #162
Conversation
Current coverage is 97.23% (diff: 94.59%)@@ master #162 diff @@
==========================================
Files 3 3
Lines 339 325 -14
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 329 316 -13
+ Misses 10 9 -1
Partials 0 0
|
|
I have a few syntax proposals to share: The imperative styleJSON.begin_array(io)
JSON.show_element(io, elt)
JSON.show_element(io, elt)
JSON.show_element(io, elt)
JSON.end_array(io)This one is not my favourite. It's too clunky and verbose, and too error-prone. Obviously the other proposals are going to end up compiling down to this, but I seriously hope we can find a better API. The context styleJSON.array(io) do
JSON.show_element(io, elt)
JSON.show_element(io, elt)
JSON.show_element(io, elt)
endThis one is a little nicer to look at than the last one (for one, it nests properly!), but it's still error-prone. It's also potential compilation overhead. The cute styleJSON.array(io) do show
show(elt)
show(elt)
show(elt)
endThis is clean and least error-prone (I can't accidentally use the wrong version of The cute style, macro versionJSON.@arr begin
show(elt)
show(elt)
show(elt)
endAvoids compilation overhead of extra functions, but it's a little more magical. As with earlier, any helper functions will need to be passed both Any other suggestions for syntax are welcome. |
|
I think I will implement the context style, with support for the imperative style as a documented API. |
TotalVerb
changed the title
|
After playing around with several approaches, I think I may as well implement #165 in this PR. There's no point implementing something halfway only to rip it apart when an implementation of #165 is needed. To implement #165, I am looking to adopt a system analogous to
I settled on this strategy primarily because of consistency with
The overarching goals are:
A few test cases for the new generality (I might implement these as tests, once the underlying implementation is done):
|
|
An alternative approach is to combine the |
|
For the "cute" version, I imagine you could do something like the following with call-overloading to pass around the IO object in a way that should be low overhead. We considered doing something like this for array(json) do json
json(elt)
json(elt)
json(elt)
end
type JSON{T <: IO} <: IO
io::T
parent::JSON{T}
end
array(f, json) = print(json.io, "[", f(json), "]")
(json::JSON)(obj) = print(json.io, '"', escape(repr(obj)), '"')
(json::JSON)(arr) = array(body -> for elt in arr; body(elt); end, json)I'm not sure I understand the purpose of |
|
Thanks for your comments, @vtjnash. Passing around the IO argument does seem like an interesting idea. Since this PR is getting quite big, I think I will implement a minimal imperative interface and then we can discuss the higher-level abstractions later. On |
|
Hi @vtjnash , I like that idea too. Any other downsides besides churn? I am building something like a baby virtual DOM in Pages.jl https://github.com/EricForgy/Pages.jl/blob/master/src/api.jl#L66-L79 I'm passing |
|
The other downside is that you only really get one verb, and the name is picked by the user (since it is the argument to the do block on the above example). But it mostly also just shows that there's nothing all that privileged about the first argument - it's symmetric multi-dispatch after all. |
TotalVerb
changed the title
|
This is open for review now. According to codecov, this is actually a net reduction in lines of code 😃. I did have to strip out the |
| # issue #168: Print NaN and Inf as Julia would | ||
| immutable NaNSerialization <: CS end | ||
| JSON.show_json(io::SC, ::NaNSerialization, f::AbstractFloat) = | ||
| Base.print(io, f) |
There was a problem hiding this comment.
Very nice, It's hard to imagine custom support for NaN and Inf being simpler than this :-)
|
Bumping this. |
| be printed; otherwise there will be no trailing newline. | ||
| """ | ||
| function show_json(io::IO, s::Serializations.Serialization, obj; indent=nothing) | ||
| ctx = indent === nothing ? CompactContext(io) : PrettyContext(io, indent) |
There was a problem hiding this comment.
The previous version, using State{true} or State{false}, was type unstable. This one is actually not (in the sense that the type of ctx only depends on the type of indent, instead of its value). However, the keyword argument sorter is type unstable anyway.
There was a problem hiding this comment.
In a non-kwargs function, it's type stable:
julia> f(x=nothing) = x === nothing ? 1 : 1.0
f (generic function with 2 methods)
julia> @code_warntype f(nothing)
Variables:
#self#::#f
x::Void
Body:
begin
(x::Void === Main.nothing)::Bool
return 1
end::Int64
julia> @code_warntype f(1)
Variables:
#self#::#f
x::Int64
Body:
begin
(x::Int64 === Main.nothing)::Bool
goto 3
3:
return 1.0
end::Float64
|
Is there anything preventing this from being merged and tagged? I want to use the JS function literals from #170, but I can't without being able to set a minimum JSON package value in REQUIRES |
|
@randyzwitch Because of the scope of this change, I'd like at least one review. Bumping this again. |
|
To slightly simplify review, I split off #177 (which is a non-functional refactoring) into a separate PR. |
|
Sorry for letting this stagnate so long. I've rebased it. |
|
I would like to move forward on this. Since this does not currently affect any observable behaviour, and since it results in a performance improvement, I plan to merge it in about a week or so unless there are any objections. |
|
Let's do this. |
This fixes the major issues with overriding the old
_writejson(née_print):_printtended to use low-level implementation details, such asJSON.separator. These packages broke when the low-level implementation changed. This proposal will introduce high-level constructs with a stable, documented API.show_json.JSONin can be considered in the future, and packages overloadingshow_jsonwill correctly use those extensions automatically.What is not covered in this PR is a nicer interface for things. That will come later.
Benchmark results (compared to before #151 was merged; #151 introduced some memory regressions) are looking good so far: