Add JSON output support to Emboss text format #237
Conversation
| @@ -1,17 +1,3 @@ | |||
| // Copyright 2019 Google LLC | |||
There was a problem hiding this comment.
Unless Google has change its policies in the last few months, this header should stay.
| */ | ||
| #ifndef ${header_guard} | ||
| #define ${header_guard} | ||
| #include <stdint.h> |
There was a problem hiding this comment.
Either delete if these are truly unnecessary, or uncomment if they're used in here.
| // #include <utility> | ||
|
|
||
| #include "runtime/cpp/emboss_cpp_util.h" | ||
| #include "third_party/emboss/runtime/cpp/emboss_cpp_util.h" |
There was a problem hiding this comment.
This is an internal-ism. Copybara will adjust the path at import.
| emboss_reserved_local_stream->Write(","); | ||
| if (!emboss_reserved_local_field_options.json() || | ||
| emboss_reserved_local_wrote_field) { | ||
| emboss_reserved_local_stream->Write(" "); |
There was a problem hiding this comment.
You have moved the prefix Write(" ") so that it no longer occurs for the first element in a structure. I don't see how the SkippedStructureFieldOutput is still passing.
Also: the Write(" ") here does not happen for JSON, but there are spaces between fields in the JSON goldens in the new tests.
Did I miss something, or is there version skew between the test and implementation?
| "{\"one_byte_enum\": \"ZERO\", \"seven_bit_uint\": 1, \"one_bit_flag\": " | ||
| "false, \"one_byte_uint\": 2, \"two_byte_uint\": 1027, " | ||
| "\"four_byte_uint\": 134678021, \"eight_byte_uint\": " | ||
| "1157159078456920585, \"uint8_array\": [17, 18, 19, 20, 21, 22, 23, 24, " |
There was a problem hiding this comment.
So: a very "fun" thing about JSON is that all numeric values are 64-bit floating-point values. The "eight_byte_uint" value here will not be parsed correctly by many JSON parsers, depending how closely they hew to the JSON specification.
The typical workaround for this is to serialize integers as base 10 ASCII strings, either:
- All integers are serialized as strings (this is what Emboss does internally for the serialized IR)
- Integers whose range is larger than +/-2^53 are serialized as strings (so any 8-byte integer would be serialized as a string, no matter what its current value)
- Integers whose actual value is outside of [-2^53,+2^53] are serialized as strings (so an 8-byte integer might be serialized as a string or a double, depending on its current value)
I don't think there is a single correct answer here, so it might just need another config value.
| (void)emboss_reserved_local_wrote_field; | ||
| if (emboss_reserved_local_options.multiline()) { | ||
| if (emboss_reserved_local_wrote_field && | ||
| emboss_reserved_local_options.json()) { |
There was a problem hiding this comment.
You can delete the && ...json() here and then lines 348-351 entirely, I believe.
| } | ||
| emboss_reserved_local_stream->Write("${field_name}: "); | ||
| if (emboss_reserved_local_field_options.json()) { | ||
| emboss_reserved_local_stream->Write("\"${field_name}\": "); |
There was a problem hiding this comment.
Most JSON serializers will omit the space between : and the field value when generating compact output.
| bool digit_grouping() const { return digit_grouping_; } | ||
| bool comments() const { return comments_; } | ||
| ::std::uint8_t numeric_base() const { return numeric_base_; } |
There was a problem hiding this comment.
These three should probably return ... && !json_ instead of having if (comments() && !json()) and similar logic scattered around the code.
2 participants
Add JSON output support to Emboss text format