-
-
Notifications
You must be signed in to change notification settings - Fork 765
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
Allowed Enum variants to be individually marked as untagged #2403
Conversation
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.
Please also add some tests for non-recursive examples like in #1402 (comment) or generic examples like #1402 (comment)
fixed spelling Co-authored-by: Oli Scherer <github35764891676564198441@oli-obk.de>
# Conflicts: # serde_derive/src/internals/attr.rs
… to fn -> optional
Thanks for all the feedback. I've tried to implement the suggestions. |
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.
one thing I just realized that we may want to reject is marking all enum variants as untagged, as then you could instead mark the entire enum as untagged.
lgtm otherwise. I'll ping a few folk from the issue so they can try out your PR and see if it works for their use cases.
I wasn't sure what the policy was on warning-like cases. Other cases to consider might be:
|
.collect() | ||
}).inspect(|variant| { | ||
if !variant.attrs.untagged() && seen_untagged { | ||
cx.error_spanned_by(&variant.ident, "all variants with the #[serde(untagged)] attribute must be placed at the end of the enum") |
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.
Why only allowing #[serde(untagged)]
to be at the end?
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 keeps the implementation more consistent with the deserialisation strategy of going though each variant in order and choosing the first variant that matches.
@oli-obk Hi! This feature is really useful, but sadly there is no activity on PR more than a month. The PR itself looks quite finished except warnings. Can it be merged in this state? |
Unfortunately no one on the issue has so far tested out this branch to see if it works for their use cases. |
@oli-obk I tried to check it, but don't know how to replace serde dependency in serde_json |
Someone was so kind and wrote instructions at #1402 (comment) |
@oli-obk My case was covered too. #1402 (comment) |
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.
Thanks!
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [serde](https://serde.rs) ([source](https://github.com/serde-rs/serde)) | dependencies | patch | `1.0.162` -> `1.0.164` | --- ### Release Notes <details> <summary>serde-rs/serde</summary> ### [`v1.0.164`](https://github.com/serde-rs/serde/releases/tag/v1.0.164) [Compare Source](serde-rs/serde@v1.0.163...v1.0.164) - Allowed enum variants to be individually marked as untagged ([#​2403](serde-rs/serde#2403), thanks [@​dewert99](https://github.com/dewert99)) ### [`v1.0.163`](https://github.com/serde-rs/serde/releases/tag/v1.0.163) [Compare Source](serde-rs/serde@v1.0.162...v1.0.163) - Eliminate build script from serde_derive crate to slightly reduce build time ([#​2442](serde-rs/serde#2442), thanks [@​taiki-e](https://github.com/taiki-e)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS45Ni4yIiwidXBkYXRlZEluVmVyIjoiMzUuMTE0LjIiLCJ0YXJnZXRCcmFuY2giOiJkZXZlbG9wIn0=--> Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1896 Reviewed-by: crapStone <crapstone@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
This should hopefully resolve #1402.
This can be thought of as applying the following transformation
to
The actual implementation generates the implementation directly allowing it to also work efficiently for recursive enums.
The current version includes a restriction that all variants marked as
untagged
appear at the end of the enum (after the tagged variants) in order to keep the implementation strategy consistent with an alternate strategy of attempting to deserialize each variant in order.Deserialize could also be implemented to accept all of the variants with tags, only falling back to untagged if none of them match, I'm not sure if this would be preferred.