-
Notifications
You must be signed in to change notification settings - Fork 101
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: 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)
end This 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)
end This 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)
end Avoids 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. |
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't ctx
type-unstable?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
):_print
tended 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
.JSON
in can be considered in the future, and packages overloadingshow_json
will 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: