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

Test #[serde(flatten)] support #50

Merged
merged 3 commits into from
Nov 29, 2022
Merged

Test #[serde(flatten)] support #50

merged 3 commits into from
Nov 29, 2022

Conversation

webmaster128
Copy link
Member

Closes #20

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Nice test to show this works.

But, it would be good to ensure it works for the particular case we would use in CosmWasm... enums flattening their variants (which are other enums).

I hope this works, but may bring up some unreachable!() instruction it would be good to flag

total: u64,
}

#[derive(Serialize, Deserialize, Debug, PartialEq, Eq)]
Copy link
Member

Choose a reason for hiding this comment

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

This is a great test case. But it would be good to have one or two for the specific version we would use in CosmWasm.

struct ExecMsg1 {
  something: String,
}

struct ExecMsg2 {
  other: u64,
}

/// one example to test
enum ExecMsg {
  #[serde(flatten)]
  One(ExecMsg1),
  #[serde(flatten)]
  Two(ExecMsg2),
}

/// another example to test
enum ExecMsg {
  #[serde(flatten)]
  One(ExecMsg1),
  // important to leave top level along with a flattened variant
  SetAge { age: u32 },
  // also interesting to see if we can use this format with them (unflattened) 
  Custom(CustomData),
}

You can make it a bit nicer than that, but combining a cw20 ExecMsg and my contracts custom one is a clear case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, right. I can easily make the flatten test for enums instead of structs. This is the part we need for CosmWasm.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out flatten is not for enums (see feature request).

So this PR would close the ticket and if we hit new issues, we can create more refined ones.

Copy link
Member

Choose a reason for hiding this comment

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

Great link.
Yes, we need untagged for that, which is another issue #43

This PR closes the linked issue well

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

lgtm (comments already addressed)

@webmaster128 webmaster128 merged commit a4b36e7 into main Nov 29, 2022
@webmaster128 webmaster128 deleted the test-flatten branch November 29, 2022 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serialization of flatten type not supported
2 participants