-
Notifications
You must be signed in to change notification settings - Fork 742
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 ability to use #[serde(default)]
to deserialize internally tagged enums when no tag is provided
#2476
base: master
Are you sure you want to change the base?
Add ability to use #[serde(default)]
to deserialize internally tagged enums when no tag is provided
#2476
Changes from 5 commits
5733520
393b0d6
7d60f0f
62023a9
2030cd5
c6c6b17
d5ce33c
8d9d374
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -805,16 +805,22 @@ mod content { | |
/// Not public API. | ||
pub struct TaggedContentVisitor<'de, T> { | ||
tag_name: &'static str, | ||
default_variant: Option<T>, | ||
expecting: &'static str, | ||
value: PhantomData<TaggedContent<'de, T>>, | ||
} | ||
|
||
impl<'de, T> TaggedContentVisitor<'de, T> { | ||
/// Visitor for the content of an internally tagged enum with the given | ||
/// tag name. | ||
pub fn new(name: &'static str, expecting: &'static str) -> Self { | ||
pub fn new( | ||
name: &'static str, | ||
default_variant: Option<T>, | ||
expecting: &'static str, | ||
) -> Self { | ||
TaggedContentVisitor { | ||
tag_name: name, | ||
default_variant: default_variant, | ||
expecting: expecting, | ||
value: PhantomData, | ||
} | ||
|
@@ -851,11 +857,12 @@ mod content { | |
where | ||
S: SeqAccess<'de>, | ||
{ | ||
let tag = match try!(seq.next_element()) { | ||
let tag = match seq.next_element()? { | ||
Some(tag) => tag, | ||
None => { | ||
return Err(de::Error::missing_field(self.tag_name)); | ||
} | ||
None => match self.default_variant { | ||
Some(variant) => variant, | ||
None => return Err(de::Error::missing_field(self.tag_name)), | ||
}, | ||
}; | ||
let rest = de::value::SeqAccessDeserializer::new(seq); | ||
Ok(TaggedContent { | ||
|
@@ -885,7 +892,13 @@ mod content { | |
} | ||
} | ||
match tag { | ||
None => Err(de::Error::missing_field(self.tag_name)), | ||
None => match self.default_variant { | ||
Some(default) => Ok(TaggedContent { | ||
tag: default, | ||
content: Content::Map(vec), | ||
}), | ||
None => Err(de::Error::missing_field(self.tag_name)), | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. |
||
Some(tag) => Ok(TaggedContent { | ||
tag: tag, | ||
content: Content::Map(vec), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -156,6 +156,18 @@ fn enum_from_ast<'a>( | |
}) | ||
.collect(); | ||
|
||
let mut has_default = false; | ||
for variant in &variants { | ||
if !variant.attrs.default() { | ||
continue; | ||
} | ||
if has_default { | ||
cx.error_spanned_by(&variant.ident, "only one variant can be marked as default"); | ||
break; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not break after the first error so all errors can be found at once |
||
has_default = true; | ||
} | ||
|
||
let index_of_last_tagged_variant = variants | ||
.iter() | ||
.rposition(|variant| !variant.attrs.untagged()); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -812,6 +812,117 @@ fn test_internally_tagged_enum() { | |
); | ||
} | ||
|
||
#[test] | ||
fn test_internally_tagged_enum_with_default_variant() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the best way to test the macro? I had a dig around the codebase, and I felt it made sense, but unsure. |
||
#[derive(Debug, PartialEq, Serialize, Deserialize)] | ||
struct Newtype(BTreeMap<String, String>); | ||
|
||
#[derive(Debug, PartialEq, Serialize, Deserialize)] | ||
struct Struct { | ||
f: u8, | ||
} | ||
|
||
#[derive(Debug, PartialEq, Serialize, Deserialize)] | ||
#[serde(tag = "type")] | ||
enum InternallyTaggedWithDefaultStructVariant { | ||
#[serde(default)] | ||
A { | ||
a: u8, | ||
}, | ||
E(Struct), | ||
B, | ||
C(BTreeMap<String, String>), | ||
D(Newtype), | ||
} | ||
|
||
#[derive(Debug, PartialEq, Serialize, Deserialize)] | ||
#[serde(tag = "type")] | ||
enum InternallyTaggedWithDefaultTupleVariant { | ||
A { | ||
a: u8, | ||
}, | ||
#[serde(default)] | ||
E(Struct), | ||
B, | ||
C(BTreeMap<String, String>), | ||
D(Newtype), | ||
} | ||
|
||
#[derive(Debug, PartialEq, Serialize, Deserialize)] | ||
#[serde(tag = "type")] | ||
enum InternallyTaggedWithDefaultUnitVariant { | ||
A { | ||
a: u8, | ||
}, | ||
E(Struct), | ||
#[serde(default)] | ||
B, | ||
C(BTreeMap<String, String>), | ||
D(Newtype), | ||
} | ||
|
||
#[derive(Debug, PartialEq, Serialize, Deserialize)] | ||
#[serde(tag = "type")] | ||
enum InternallyTaggedWithDefaultMapVariant { | ||
A { | ||
a: u8, | ||
}, | ||
E(Struct), | ||
B, | ||
#[serde(default)] | ||
C(BTreeMap<String, String>), | ||
D(Newtype), | ||
} | ||
|
||
#[derive(Debug, PartialEq, Serialize, Deserialize)] | ||
#[serde(tag = "type")] | ||
enum InternallyTaggedWithDefaultNewtypeVariant { | ||
A { | ||
a: u8, | ||
}, | ||
E(Struct), | ||
B, | ||
C(BTreeMap<String, String>), | ||
#[serde(default)] | ||
D(Newtype), | ||
} | ||
|
||
assert_de_tokens( | ||
&InternallyTaggedWithDefaultStructVariant::A { a: 1 }, | ||
&[ | ||
Token::Map { len: Some(1) }, | ||
Token::Str("a"), | ||
Token::U8(1), | ||
Token::MapEnd, | ||
], | ||
); | ||
|
||
assert_de_tokens( | ||
&InternallyTaggedWithDefaultTupleVariant::E(Struct { f: 1 }), | ||
&[ | ||
Token::Map { len: Some(1) }, | ||
Token::Str("f"), | ||
Token::U8(1), | ||
Token::MapEnd, | ||
], | ||
); | ||
|
||
assert_de_tokens( | ||
&InternallyTaggedWithDefaultUnitVariant::B, | ||
&[Token::Map { len: Some(0) }, Token::MapEnd], | ||
); | ||
|
||
assert_de_tokens( | ||
&InternallyTaggedWithDefaultMapVariant::C(BTreeMap::new()), | ||
&[Token::Map { len: Some(0) }, Token::MapEnd], | ||
); | ||
|
||
assert_de_tokens( | ||
&InternallyTaggedWithDefaultNewtypeVariant::D(Newtype(BTreeMap::new())), | ||
&[Token::Map { len: Some(0) }, Token::MapEnd], | ||
); | ||
} | ||
|
||
#[test] | ||
fn test_internally_tagged_bytes() { | ||
#[derive(Debug, PartialEq, Deserialize)] | ||
|
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.
I think, it is better to use
.or(self.default_variant)
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're right, that's much nicer :)