-
Notifications
You must be signed in to change notification settings - Fork 231
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 a default case for impl_deserialize_for_internally_tagged_enum
#634
Add a default case for impl_deserialize_for_internally_tagged_enum
#634
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #634 +/- ##
=======================================
Coverage 61.24% 61.24%
=======================================
Files 39 39
Lines 16277 16277
=======================================
Hits 9969 9969
Misses 6308 6308
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Good, but small improvements are possible. Also, please update Changelog.md
: add new entry under Features (links to issues are listed at the end of version section). Link it to the issue #629, not to this PR.
src/serde_helpers.rs
Outdated
$( _ => Ok(deserialize_variant!( de, $enum, $($default_variant)+ )), )? | ||
#[allow(unreachable)] |
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.
You can use intermediate macro with two possible variants to select required behavior. General idea:
macro_rules! def {
// variant 1
(
$($variant_tag:literal),*
_ => Ok( $tokens:tt )
) => {
_ => Ok($tokens),
},
// variant 2
(
$($variant_tag:literal),*
) => {
_ => Err(A::Error::unknown_field(&tag, &[$($variant_tag),+])),
},
}
// use
def!(
$($variant_tag),*
$( _ => Ok(deserialize_variant!( de, $enum, $($default_variant)? )), )?
)
If $(...)?
part exists, then the first alternative would be selected, otherwise the second. That way we would not need to use #[allow(unreachable)]
attribute, which could be forbidden in the project via #[forbid(unreachable)]
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 for the pointer, I had the conviction the allow_unreachable wasn't a clean solution.
I get the general idea here, but this code doesn't compile ("no rules expected the token ,
") and I have no clue what I should change.
Thanks.
Closest I got:
#[macro_export]
#[doc(hidden)]
macro_rules! def {
// variant 1
(
$($variant_tag:literal),*
_ => Ok($($tokens:tt)*)
) => {
_ => Ok($($tokens)*),
};
// variant 2
(
$($variant_tag:literal),*
) => {
_ => Err(A::Error::unknown_field(&tag, &[$($variant_tag),+])),
}
}
match tag.as_ref() {
$(
$variant_tag => Ok(deserialize_variant!( de, $enum, $($variant)+ )),
)*
quick_xml::def!(
$($variant_tag),*
$( _ => Ok(deserialize_variant!( de, $enum, $($default_variant)? )), )?
)
}
… macro Co-authored-by: Mingun <[email protected]>
b5ad621
to
421dc0f
Compare
Thanks for your work! I implemented the helper macro myself and push it. |
Attempt to implement #629 .
,
at the end of the list of all variants so I replicated this behaviour. But I think in the current version, it'd accept 2 commas, not sure if what I wrote is the cleanest.