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

Adds support for parameterized Rust enums in serde #733

Merged
merged 3 commits into from
Apr 3, 2024
Merged

Conversation

desaikd
Copy link
Contributor

@desaikd desaikd commented Apr 1, 2024

Description of changes:
This PR works on adding support for parameterized Rust enums with experimental-serde feature.

Example:
Below is an example parameterized enum value in Rust:

#[serde_as]
#[derive(Serialize, Deserialize)]
enum ParameterizedEnum {
    Unit,
    Newtype(u32),
    Tuple(u32, u32),
    Struct { a: u32 },
}

Ion representation:

// For each of the enum variant following are corresponding Ion representation
Unit // simply a symbol value 
NewType::1
Tuple::[1, 2]
Struct::{a: 1}

List of changes:

  • Modified mapping table for serde data types to Ion types.
  • Modified serialization to remove adding a new struct for newtype_variant, struct_variant and tuple_variant. Instead add annotation with variant name.
  • Modified deserialization to check for annotation with newtype_variant, struct_variant and tuple_variant.

Test:
Added tests for enum variants and struct types.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@desaikd desaikd requested a review from zslayton April 1, 2024 19:27
}
}

fn deserialize_identifier<V>(self, visitor: V) -> Result<V::Value, Self::Error>
where
V: Visitor<'de>,
{
visitor.visit_str(self.value.read()?.expect_text()?)
// annotations are currently only supported with parameterized Rust enums
Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly does this mean? Does the rustdoc for this feature need to be updated as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This means that a serialized Ion data will only contain annotation in the case of parameterized Rust enums. And yes, I did add more comments in the rustdocs for it updating the table of serde data type to Ion type mappings: https://github.com/amazon-ion/ion-rust/pull/733/files#diff-de73824c75ee6203daf57ccbc5aee2ab3e2b46fbd9219d494c190298a7a8e745R43

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I saw that but didn't make the connection. It might be worth renaming the table to say "Ion representation" instead of "Ion data type" now that we're including more information than just the data type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I will change that.

Copy link
Contributor

@popematt popematt left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll defer to @zslayton in case he has anything else to add.

@desaikd desaikd merged commit 55f0614 into main Apr 3, 2024
28 checks passed
@desaikd desaikd mentioned this pull request Apr 4, 2024
3 tasks
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.

3 participants