JSON formatting in C# #582
Conversation
- No parsing - Reflection based, so not hugely efficient - No line breaks or indentation
- Handle oneof properly - Omit unknown enum values
|
@jskeet how do you plan to support Any message in the JSON conversion? |
|
Unsure as yet. I'm going to do the easier well-known types (Timestamp, Duration, Struct etc first), and hope that Java has tackled this before I have to... |
| } | ||
|
|
||
| // Now oneofs | ||
| foreach (var accessor in fields.Oneofs) |
There was a problem hiding this comment.
So all the one ofs will appear in the JSON representation after all the other fields? Isn't that a bit odd? To me it looks like the fields should be written ordered by their field numbers.
There was a problem hiding this comment.
Well, that code would be considerably uglier. I can try it if you want, but bearing in mind that the field numbers aren't even visible in the JSON, and oneofs already behave a bit weirdly in JSON, I think I'd prefer to keep it like this. I would expect different implementations to make different decisions on this, when it comes to ordering. (I'm sure I saw something somewhere saying that the ordering didn't matter for JSON, but I can't find it now.)
There was a problem hiding this comment.
I think it's fine to leave it like this but I'd still add a note or a todo, because the ordering is not ideal currently (even though it's allowed by proto3).
|
The code LGTM after addressing the minor issues mentioned above (adding a TODO and removing the unnecessary indexers). Feel free to merge afterwards. |
- Remove the indexers in FieldAccessorTable - Add a TODO for field ordering in oneof
(No parsing is supported yet. Some extra reflection support was required for oneof.