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.

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

Why Token::Enum + smth and Token::...Variant is exist? #1

Open
Mingun opened this issue Apr 23, 2023 · 2 comments
Open

Why Token::Enum + smth and Token::...Variant is exist? #1

Mingun opened this issue Apr 23, 2023 · 2 comments

Comments

@Mingun
Copy link
Contributor

Mingun commented Apr 23, 2023

I found, that there are two ways to express how the enum should be serialized:

#[derive(Deserialize, Serialize, Debug, PartialEq)]
enum Enum {
  Unit,
}

assert_tokens(
  &Enum::Unit,
  &[
    Token::Enum { name: "Enum" },
    Token::Str("Unit"),
    Token::Unit,
  ],
);

assert_tokens(
  &Enum::Unit,
  &[
    Token::UnitVariant { name: "Enum", variant: "Unit" },
  ],
);

Surprisely, both of that asserts are passed, although is is obvious that serialization can give only one of that results. After examining code I've found this very confusing check:
https://github.com/serde-rs/serde/blob/0c6a2bbf794abe966a4763f5b7ff23acb535eb7f/serde_test/src/ser.rs#L184-L190
That means that tester will change it's behavior depending on the testing data, that is not expected from the testing tool.

So the questions are:

  • why both variants are exist?
  • what one should use?
  • isn't it necessary to keep only one of them?
@dtolnay
Copy link
Member

dtolnay commented Jul 9, 2023

I think this came from the Serializer vs Deserializer API having slightly divergent representation of the Serde data model out of necessity. In particular, enums during deserialization are treated using deserialize_enum and visit_enum (this is analogous to Token::Enum), while during serialization there is no such thing as "enum", only serialize_{…}_variant (this is analogous to Token::…Variant).

When one is testing serialization only, or deserialization only, one or the other way of expressing the enum using Token may seem more appropriate. I wouldn't say it's necessary for serde_test to have both.

I'll go ahead and close this because the current situation seems fine to me, but we should feel free to scrutinize it again for 2.x or if someone is building a different serde testing library.

@dtolnay dtolnay closed this as completed Jul 9, 2023
@Mingun
Copy link
Contributor Author

Mingun commented Jul 10, 2023

Seems reasonable. I think, that this subtle difference with explanation should be documented on Token::Enum and have are references to that documentation in Token::...Variant also would be good. Unfortunately, I'm not sure that I can write good documentation because of my language. So if you could add a few lines, it would be great!

@dtolnay dtolnay reopened this Jul 10, 2023
@dtolnay dtolnay transferred this issue from serde-rs/serde Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants