-
Notifications
You must be signed in to change notification settings - Fork 15
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
Conversation
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.
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)] |
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.
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.
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.
Yeah, right. I can easily make the flatten test for enums instead of structs. This is the part we need for CosmWasm.
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.
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.
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.
Great link.
Yes, we need untagged for that, which is another issue #43
This PR closes the linked issue well
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.
lgtm (comments already addressed)
Closes #20