Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix internally tagged enums by implementing ContentDeserializer::deserialize_enum #787

Merged
merged 5 commits into from
Feb 23, 2017
Merged

Fix internally tagged enums by implementing ContentDeserializer::deserialize_enum #787

merged 5 commits into from
Feb 23, 2017

Conversation

46bit
Copy link
Contributor

@46bit 46bit commented Feb 23, 2017

These commits are to fix #775 for internally tagged enums. My testcases pass with this implementation but I'm not sure about the code correctness. Is there a particular test file I should add to?

Untagged enums need a similar fix.

@46bit
Attempt to port serde_json::Value::deserialize_enum for ContentDeseri…
4e665a7 
…alizer is being derailed by what error types should be used.
@46bit
Resolved error typing. Needs to adapt to Content::Map with non-String…
8f6e1fb 
… keys.

`EnumDeserializer` is being adapted from:
  `serde_json::Value::Map<Vec<(String, Value)>>`
serde has a Map variant that allows non-String keys:
  `serde::de::Content::Map<Vec<(Content, Content)>>`

There's a lot of assumptions in `EnumDeserializer` about `String` keys and I'm not sure what the adaptation should be.
@46bit
Compiles and fixes internally tagged testcases.
4c25374
46bit added a commit to sirpent-team/sirpent-rust that referenced this pull request Feb 23, 2017
@46bit
Adopted {"kind": "Msg variant", "data": {...}} Msg format.
3e1e572 
Uses my (unfinished) serde-rs/serde#787 fix to internally tagged enums.
@dtolnay
Copy link
Member

dtolnay commented Feb 23, 2017

This PR is great. Thanks so much for working through this. I added two tiny commits to get the build to pass.

You or someone else can follow up in a separate PR for ContentRefDeserializer and tests before we close #775. For tests I would want a new function at the bottom of test_suite/tests/test_macros.rs dedicated to this case.

@dtolnay dtolnay merged commit 8e5f472 into serde-rs:master Feb 23, 2017
@dtolnay
Copy link
Member

dtolnay commented Feb 23, 2017

More specifically, the test should have an internally tagged enum with one variant containing an externally (default) tagged enum with four variants, unit + newtype + tuple + struct. The test should use assert_tokens like the other tests in that file to make sure each case can be serialized and deserialized as expected.

@46bit
Copy link
Contributor Author

46bit commented Feb 23, 2017

Cheers! I was wondering whether we should make EnumDeserializer et al type-parametric in whether they use ContentDeserializer or ContentRefDeserializer internally. This would save 200 lines of code duplication. What do you think?

@dtolnay
Copy link
Member

dtolnay commented Feb 23, 2017

Sure, give it a try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error when deserialising an enum within a tagged enum.
2 participants
@46bit @dtolnay