add missing visit_enum #2836
add missing visit_enum #2836
Conversation
|
@dtolnay is there an issue with this request? |
| let (key, data) = tri!(visitor.variant::<String>()); | ||
| Ok(Content::Map( | ||
| [( | ||
| Content::String(key), | ||
| tri!(data.newtype_variant::<Self::Value>()), | ||
| )] | ||
| .into(), |
There was a problem hiding this comment.
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.
|
I can confirm this PR fixes surrealdb/surrealdb#4844 currently stuck with untagged variant deserialize by adding this method code into crate it works as expected without this method untagged enum with input data doesn’t deserialize and throws an error when will this PR be merged? |
fixes surrealdb/surrealdb#4844 and probably dtolnay/serde-yaml#344