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

add missing visit_enum #2836

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

add missing visit_enum #2836

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6fefeb1
add missing visit_enum
frederik-uni Oct 18, 2024
1286b8a
fix action
frederik-uni Oct 18, 2024
fcf0224
Merge branch 'serde-rs:master' into master
frederik-uni Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions serde/src/private/de.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ mod content {
use crate::de::value::{MapDeserializer, SeqDeserializer};
use crate::de::{
self, size_hint, Deserialize, DeserializeSeed, Deserializer, EnumAccess, Expected,
IgnoredAny, MapAccess, SeqAccess, Unexpected, Visitor,
IgnoredAny, MapAccess, SeqAccess, Unexpected, VariantAccess, Visitor,
};

/// Used from generated code to buffer the contents of the Deserializer when
Expand Down Expand Up @@ -525,12 +525,17 @@ mod content {
Ok(Content::Map(vec))
}

fn visit_enum<V>(self, _visitor: V) -> Result<Self::Value, V::Error>
fn visit_enum<V>(self, visitor: V) -> Result<Self::Value, V::Error>
where
V: EnumAccess<'de>,
{
Err(de::Error::custom(
"untagged and internally tagged enums do not support enum input",
let (key, data) = tri!(visitor.variant::<String>());
Ok(Content::Map(
[(
Content::String(key),
tri!(data.newtype_variant::<Self::Value>()),
)]
.into(),
Comment on lines +532 to +538
Copy link
Contributor

@Mingun Mingun Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that you should at least use Content instead of String, and probably introduce a new variant in Content for enum.

Also, need an explanation, why you choose newtype_variant? If that the only thing that expected when deserializing untagged and internally tagged enums, then this should be explicitly explained, because this is not obvious at all.

The last, but not least, you should add tests that cover this change. I tried to bring some hierarchial structure to the serde tests so that the new contributors know where to start, but not all PRs are accepted yet. However, for that part tests seems, already concentrated in one place:

I think that adding those details will increase chance of the PR to be reviewed by maintainers.

))
}
}
Expand Down