Skip to content
This repository has been archived by the owner on Aug 15, 2021. It is now read-only.

Adds the option to encode enums as 1-key maps. #67

Merged
merged 7 commits into from
Dec 19, 2018

Conversation

ogoodman
Copy link
Contributor

@ogoodman ogoodman commented Jun 7, 2018

Since enums are part of the Rust language but not part of the CBOR or JSON spec, each serializer has to make a choice how to encode them.

serde_cbor chose ["Variant", items..] while serde_json chose {"Variant": item} or {"Variant": [items..]}. Although this is immaterial when passing data between Rust programs, it becomes visible if Values are used, or when data is passed between Rust and other languages.

This turned out to be a problem for me when I tried to migrate a communicating pair of Python and Rust programs from JSON to CBOR, as assumptions about the encoding of enums had already become buried throughout the Python code.

This pull request adds the option (default false) to encode enums in the serde_json way and adds support for automatically decoding enums from either encoding.

@kostko
Copy link
Contributor

kostko commented Nov 21, 2018

This would indeed be very useful to have, I also have a use case that would be much easier to support from a Go serializer with something like this PR is proposing. Are there any design decisions needed, can anyone review?

@pyfisch
Copy link
Owner

pyfisch commented Nov 21, 2018

Sorry. I missed this. I will review this week.

Copy link
Contributor

@kostko kostko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to use the code from this PR and there are a few issues that need to be fixed:

  • Unit variants are still serialized as string/int even if enum_as_map is set. To be consistent, they should be serialized as maps as well, e.g., {"Foo": {}} as otherwise deserialization in a different language needs to deal with a special case where the value is a string/int instead of a struct which is annoying.
  • Struct variants are still serialized as 2-length lists even if enum_as_map is set.

These are trivial to fix and I've fixed them in my branch - I can push my changes if needed. Otherwise the code seems to work well.

@pyfisch
Copy link
Owner

pyfisch commented Nov 21, 2018

I can push my changes if needed.

Please create a PR.

@kostko
Copy link
Contributor

kostko commented Nov 21, 2018

So for my first point, actually it seems that serde-json serializes unit variants in this same way, so perhaps for consistency sake we can leave this as is and the user just needs to convert unit variants to struct variants if different behavior is desired? Not sure about this one.

@pyfisch
Copy link
Owner

pyfisch commented Nov 23, 2018

I think if we add this the serialization should match the one used by serde-json.

Copy link
Owner

@pyfisch pyfisch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs some tests. With the comments addressed it is fine.

src/ser.rs Outdated Show resolved Hide resolved
src/ser.rs Outdated Show resolved Hide resolved
@ogoodman
Copy link
Contributor Author

Tests added. I've also added a to_vec function to SerializerOptions and implemented Default on the latter. That makes it possible for people to code in such a way that adding new options won't be a breaking change (although that requires that they add ..Default::default()). The alternative is having private fields and a bunch of builder methods, but I don't love that either.

@kostko
Copy link
Contributor

kostko commented Dec 17, 2018

@ogoodman thanks for taking the time to update this, I just got back from vacation and didn't have time to push my changes before. I did a cursory look over the new changes and it seems ok.

Regarding option defaults, either the Default way or the builder seem fine to me with a slight preference for Default due to requiring less code, especially for these simple flags that are currently implemented.

@ogoodman
Copy link
Contributor Author

@kostko thanks for your comments. Sorry if I caused you to waste time working on this. I didn't think I'd have time to work on it for a while, but then I unexpectedly did.

@pyfisch pyfisch merged commit 32b3cb2 into pyfisch:master Dec 19, 2018
@pyfisch
Copy link
Owner

pyfisch commented Dec 19, 2018

Thank you @ogoodman 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants