-
Notifications
You must be signed in to change notification settings - Fork 123
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.
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
Add minimal support for internally tagged and untagged enums #451
Add minimal support for internally tagged and untagged enums #451
Conversation
@torkleyy I really don't like that serde forces us to do this but the issue comes up again and again and a slightly hacky solution that relies on observable serde behaviour that has not changed since 1.0.0 (https://github.com/serde-rs/serde/blame/0c6a2bbf794abe966a4763f5b7ff23acb535eb7f/serde/src/private/de.rs#L220) and does not change RON's behaviour in any other way might be the best we can get for the next few years. I'd thus suggest merging this (with a heavy heart). |
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #451 +/- ##
==========================================
- Coverage 85.24% 84.72% -0.53%
==========================================
Files 62 63 +1
Lines 7830 8227 +397
==========================================
+ Hits 6675 6970 +295
- Misses 1155 1257 +102
☔ View full report in Codecov by Sentry. |
Dang, now I remember why #409 got so large ... Unfortunately supporting untagged and internally tagged enums also means needing to support self-describing newtype variants for these cases ... I also cleaned up our tuple struct detection along the way but it still becomes a bit like spaghetti code since something newtype-looking cannot just always be unwrapped since our |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a like a decent approach given our options 👍
I think we'll need some documentation. Unfortunate that these hacks are needed...
c9ca82a
to
9f5f3b4
Compare
Close acatton#14. I adapted the implementation from [ron-rs/ron#451](ron-rs/ron#451). This is a workaround for Serde's internal buffer type used when deserializing via `visit_enum`.
Close acatton#14. I adapted the implementation from [ron-rs/ron#451](ron-rs/ron#451). This is a workaround for Serde's internal buffer type used when deserializing via `visit_enum`.
Close acatton#14. I adapted the implementation from [ron-rs/ron#451](ron-rs/ron#451). This is a workaround for Serde's internal buffer type used when deserializing via `visit_enum`. Signed-off-by: Jonson Petard <41122242+greenhat616@users.noreply.github.com>
Close acatton#14. I adapted the implementation from [ron-rs/ron#451](ron-rs/ron#451). This is a workaround for Serde's internal buffer type used when deserializing via `visit_enum`. Signed-off-by: Jonson Petard <41122242+greenhat616@users.noreply.github.com>
Quite hacky but minimally invasive implementation to support internally tagged and untagged enums.
This PR switches the self-describing enum deserialisation behaviour to match serde's internal JSONy
Content
type format if we think we are deserialising into serde's internal type. Specifically, it usesstd::any::type_name
to detect whenserde::__private::de::content::Content
is deserialised to. This is relying on undocumented behaviour in all the horrible ways ... but that's what we need anyways need to do if we want to support internally tagged and untagged enums in serde. In some distant future, serde-rs/serde#2420 might make this less horrible.Unlike #409, this PR is very targeted as it does not change the deserialisation behaviour for any other types. This allows us to stick with the existing self-describing enum behaviour and gives us space to evolve
ron::Value
separately.Closes #449
CHANGELOG.md