Json.print() api in stdlib json package#223
Conversation
ponylang-main
added
status - new
mfelsche
changed the title
|
|
||
| # Summary | ||
|
|
||
| Adapt the stdlib [`json` package][json-package] in order to add means to encode arbitrary [`JsonValue`][JsonValue]s into `String` or `Array[U8]`. And while we are at it, provide a matching API for decode `String` or `Array[U8]` into [`JsonValue`] for completeness. |
There was a problem hiding this comment.
does JsonParser.parse not do what you are looking for with decode?
It takes a String and gives you a JsonValue. I'm unclear why we would wrap that.
There was a problem hiding this comment.
JsonParser.parse does exactly what i am looking for with decode. My suggestion is to have both decode and encode apis to be similar and expectable from a users point of view. They should follow the same pattern.
So it is either go with something that matches the pattern already in use with JsonParser.parse or find something new. I was going for Json.something because the JsonParser is not actually a thing that does the parsing, but delegates to JsonTokenParser hence i found it weird to call that primitive a Parser. This is just about naming things, nothing else. In the end I am quite indifferent about under what name to expose the decoding api, but since this an RFC, i wanted to come up with a reasonable decision.
|
|
||
| `JsonObject` and `JsonArray` provide methods for encoding them as valid JSON with the `.string()` and `.pretty_string()` methods, but for all other possible JSON values (string, number, bool, null) there is no such method. The `.string()` method on `None` is producing `"None"` (without the quotes), which is invalid JSON. Another example: the String `"\""` is copying itself when `.string()` is called on it, without any escaping, which is also invalid JSON. The method `.pretty_string()` is missing from all those other [JsonValue][JsonValue] instances. | ||
|
|
||
| This package needs a consistent and convenient way to encode and decode json bytes from and to all possible [JsonValue][JsonValue] instances. |
There was a problem hiding this comment.
I believe the decode already exists. What we are talking about is taking arbitrary JsonValue items and encoding to Json rather than starting from a JsonArray or JsonObject, yes?
|
|
||
| We might want to remove the `JsonObject.string()` and `JsonArray.string()` (and `.string_pretty()`) functions. As all [JsonValue][JsonValue] members implement `ToString` and thus all have a `.string()` method, but don't all produce valid JSON, this might be a source of confusion. This would also shrink the amount of different ways to achieve one thing to one: `Json.print()`. | ||
|
|
||
| The `JsonParser` primitive should also be removed and the contents of its `parse()` method should be moved to the `Json.parse()` method, to have only one way of decoding a string as JSON. |
There was a problem hiding this comment.
What happens to its current dual, JsonPathParser? This trade one asymmetry for another?
I quite like the JsonPathParser.parse() and JsonParser.parse()
Ideally I'd rather not lose that.
I think I'd veer more towards having a JsonPrinter, perhaps as you say, exposing _JsonPrint in some fashion.
I'm not quite sure but we if did something like JsonPrinter that was exposing _JsonPrint modulo a little bit of changes, would that fill your desire?
There was a problem hiding this comment.
I agree with Sean about this.
Conceptually I have no issue with "more than one way" of parsing, since I see one as just a "convenience wrapper" of the other - having a convenience wrapper that is simpler than the underlying mechanism is a common design pattern.
There was a problem hiding this comment.
We can totally go for JsonParser.parse() and JsonPrinter.print(). I don't prefer this choice, but I would go for it, if this is the preferred way. My main pain point is to have this functionality exposed.
And yes, the encoding part should expose the functionality that is already there in _JsonPrint, as shown in the first code example of the RFC.
|
|
||
| The package documentation of the `json` package should show usage examples of these functions most prominently and then show all other components of the crate, like `JsonValue`, `JsonPath` or `JsonNav`. | ||
|
|
||
| The intend of this primitive is to expose an easy-to-use interface for turning strings into structures JSON values and vice versa. This should answer the question of how to get from bytes or strings from the network or from files to structured JSON that can be analyzed (Pass the string to `Json.parse()`), and how to serialize Pony objects and data-structures as JSON (build a `JsonValue`, then pass it those `Json.print()`). |
There was a problem hiding this comment.
longer term, I'd rather be able to have a different method to serialize pony objects to json rather than having to round-trip through a JsonValue. I've been looking at general serialization options.
There was a problem hiding this comment.
This RFC is not about the longer term or a generalized serialization framework or whatnot, but about exposing JSON encoding from JsonValue to String right now. We can have the long-term discussion in another RFC or at another place any time.
The paragraph above is about how the usage of the current json package (modulo the change of this RFC) is to be documented.
SeanTAllen
added
status - new
|
@mfelsche our reading of this is that you would be good with updating to our preferred approach. is that a reasonable reading? |
ponylang-main
added
the
discuss during sync
|
@SeanTAllen Could you summarize, what your preferred approach is? Is it using What is your preference about removing |
Demonstrates the approach discussed in ponylang/rfcs#223 — expose _JsonPrint's functionality through a public JsonPrinter primitive with print() and pretty() methods, rather than the Json.print() wrapper proposed in the RFC.
|
To make things more concrete. I am imagining https://github.com/ponylang/ponyc/pull/5084/changes |
|
I am going to rephrase this RFC to add a JsonPrinter as per your draft PR and rename Side-effect of this change will be that |
|
|
||
| # How We Test This | ||
|
|
||
| Additional property-based tests will be added for encoding arbitrary [JsonValue][JsonValue]s via `JsonPrinter.print(...)` and parsing them again via `JsonParser.parse(...)` and ensuring equivalence of the initial values and the ones produces by the roundtrip. |
There was a problem hiding this comment.
what additional tests? this is loose enough that it would be hard to know when the RFC has implemented this. Do you have any thoughts here? I'd like to get a "bare minimum" into the RFC.
There was a problem hiding this comment.
I just saw that we already have such a test here: https://github.com/ponylang/ponyc/blob/main/packages/json/_test.pony#L191
So this test just needs to be updated to use the new public api and we need some more tests verifying JsonValue printing produces correct results i.e. that we don't have a common bug between encoding and decoding which makes the roundtrip tests work but actually produces invalid json (e.g. bool, ints, various floats, strings including escape sequences, such tests for arrays and objects already exist).
|
|
||
| # Drawbacks | ||
|
|
||
| Renaming `.string()` and `.string_pretty()` on `JsonObject` and `JsonArray` to `.print()` and `.pretty_print()` is a breaking change. It will cause libraries and applications using the `json` package to be changed in order to be compiled with the ponyc version containing this change. |
There was a problem hiding this comment.
Given the early age, I'm not worried about this being a breaking change.
|
|
||
| - Should a partial version of parsing be also exposed? | ||
| - Should `Json.print()` expose finer controls than just the `pretty` flag? | ||
| - Should the options, like pretty-printing or not, be exposed as optional parameters to `JsonPrinter.print()` instead of exposing `.print()` and `pretty()`? |
There was a problem hiding this comment.
Can we anticipate what other options we may want to expose?
The main one that comes to mind for me is to add configurable indentation amount/style (i.e. spaces vs tabs) to pretty-printing (and to document what the default indentation amount/style is)
Are there other options we'd want? We probably want to try to consider them for design in this RFC rather than adding more potentially-breaking changes later.
There was a problem hiding this comment.
While there is not much options to add to .print(), as its behaviour is to not insert any indents, spaces or newlines whatsoever.
The .pretty() path is adding a configurable indent. For each level of nesting, the provided indent string is repeated times the current level. Additionally, linebreaks are inserted, after each object or array member.
This could also be controlled via an exposed option like add_newlines: Bool = true, if anyone would want pretty json on a single line. Also spacings are added after each object members delimiter :.
We could distill the difference between .pretty() and .print() into three options:
- add_newlines: after the opening
[/{and after each object and array member - add_spacings: adds a whitespace after each object member delemiter
: - add_indent: adds indent based on the level of nesting
with these one could model .print() as a new common printing function:
JsonPrinter.print_with_options(value where add_newlines=false, add_spacings=false, indent="")and .pretty() could be expressed as:
// indent is two spaces
JsonPrinter.print_with_options(value where add_newlines=true, add_spacings=true, indent=" ")So with this we could have JsonPrinter.print_with_options() as above and .print() and .pretty() both forward to .print_with_options() with the above arguments added, without exposing them. I do like this approach, as .pretty() communicates the intent of getting a nice readable json and .print() is for compact single-line json. This works without knowing any internal knobs and whistles of the concrete implementation. For anyone who wants to tweak how the output should look like in detail, JsonPrinter.print_with_options() is the way to go.
If we'd say we condense it all down to just one JsonPrinter.print() function with the above arguments, we make it harder for anyone who just wants readable json.
So, if we would consider all possible knobs in the current implementation, my suggestion would now look lile this:
primitive JsonPrinter
"""
Serialize any `JsonValue` to a JSON string.
"""
fun print(value: JsonValue): String iso^ =>
"""Compact JSON serialization of any `JsonValue`."""
JsonPrinter.print_with_options(value where indent = "", add_newlines = false, add_spaces = false)
fun pretty(value: JsonValue, indent: String = " "): String iso^ =>
"""Pretty-printed JSON serialization of any `JsonValue`."""
JsonPrinter.print_with_options(value, indent where add_newlines = true, add_spaces = true)
fun print_with_options(value: JsonValue, indent: String = "", add_newlines: Bool = false, add_spaces: Bool = false): String iso^ =>
"""JSON serialization with all possible formatting options exposed. Defaults to compact formatting."""
// TBDBut this would require a slight rework of the current _JsonPrint mechanics.
There was a problem hiding this comment.
After considering it all, I would argue that .print(value) and .pretty(value, indent: String = " ") are sufficient for almost all situations, so the changes suggested above are most likely overkill and should'nt be considered, imho.
| # Unresolved questions | ||
|
|
||
| - Should the options, like pretty-printing or not, be exposed as optional parameters to `JsonPrinter.print()` instead of exposing `.print()` and `pretty()`? | ||
|
|
||
| [json-package]: https://github.com/ponylang/ponyc/tree/main/packages/json | ||
| [JsonValue]: https://github.com/ponylang/ponyc/blob/main/packages/json/json.pony#L243 |
There was a problem hiding this comment.
Would the optional parameter be an indent? IE if you give an indentation it pretty prints?
There was a problem hiding this comment.
There are several options, one I explored in this comment: #223 (comment)
I still like exposing .print() and .pretty() the most. These are rather alternatives to be considered, than open questions. I will rephrase.
|
@mfelsche I'm liking this. I have a couple questions but definitely liking. Thanks for your time on this. |
Co-authored-by: Joe Eli McIlvain <joe.eli.mac@gmail.com>
|
@mfelsche is this ready for another round of review? it appears so. "just checking". |
|
@SeanTAllen yes, please. Lets move this along. |
|
I have some slight hesitation around not being stringable. Slight. I would be willing to accept this RFC. And I am definitely ok with this going to final comment period. |
SeanTAllen
added
status - final comment period
|
To that point, I'd be in favor of having a |
SeanTAllen
added
status - ready for vote
|
I'm in favor. |
|
👍 |
|
thumbs up |
ponylang-main
removed
the
discuss during sync
The json package could parse any JSON into a JsonValue but could only serialize objects and arrays — and even those went through Stringable, which let scalars and None reach `.string()` and produce invalid JSON (None became `None`, strings went unescaped). RFC #86 closes that gap with a JsonPrinter primitive that serializes the whole JsonValue union as the dual of JsonParser. This diverges from RFC #86 in one respect: rather than renaming the object/array serializers to print()/pretty_print() and keeping them, their string()/pretty_string() methods are removed outright and Stringable is dropped. JsonPrinter is the single serialization surface, which avoids a redundant dual API and the pretty()/pretty_print() naming asymmetry. Either way this is a breaking change for json package users. Closes #5369 Design: ponylang/rfcs#223
The json package could parse any JSON into a JsonValue but could only serialize objects and arrays — and even those went through Stringable, which let scalars and None reach `.string()` and produce invalid JSON (None became `None`, strings went unescaped). RFC #86 closes that gap with a JsonPrinter primitive that serializes the whole JsonValue union as the dual of JsonParser. This diverges from RFC #86 in one respect: rather than renaming the object/array serializers to print()/pretty_print() and keeping them, their string()/pretty_string() methods are removed outright and Stringable is dropped. JsonPrinter is the single serialization surface, which avoids a redundant dual API and the pretty()/pretty_print() naming asymmetry. Either way this is a breaking change for json package users. Closes #5369 Design: ponylang/rfcs#223
The json package could parse any JSON into a JsonValue but could only serialize objects and arrays — and even those went through Stringable, which let scalars and None reach `.string()` and produce invalid JSON (None became `None`, strings went unescaped). RFC #86 closes that gap with a JsonPrinter primitive that serializes the whole JsonValue union as the dual of JsonParser. This diverges from RFC #86 in one respect: rather than renaming the object/array serializers to print()/pretty_print() and keeping them, their string()/pretty_string() methods are removed outright and Stringable is dropped. JsonPrinter is the single serialization surface, which avoids a redundant dual API and the pretty()/pretty_print() naming asymmetry. Either way this is a breaking change for json package users. Closes #5369 Design: ponylang/rfcs#223
5 participants
Rendered