Skip to content
This repository was archived by the owner on Mar 25, 2024. It is now read-only.

- Adding support for explicit tag based discriminant for Rust Enum parsing for serde_yaml #220

Closed
wants to merge 2 commits into from

Conversation

dchakrav-github
Copy link

@dchakrav-github dchakrav-github commented Nov 2, 2021

There is being started as a draft PR as there is change needed in yaml_rust implementation for propagation of tags for sequence and mapping start events. The change in yaml_rust address chyh1990/yaml-rust#147 (pending PR request filed there. Currently, Cargo.toml points to the public fork where the fix exists. This makes PR local checkout, building and inspecting the work easier), #147, that allows for explicit tags to be used to map to the Enum.

Please note, this PR explicitly choose not to address serialization with explicit tag discriminators. It isn't clear what dependency the consumers of this crate's have on the output serialization format. Can follow up with a separate PR change for Serializer change. @dtolnay seeking your advice on compatibility here.

Summary of the change:

  • Co-change needed in yaml_rust to propagate Tags as a part of Sequence/Mapping Start events to map and use for deserialization
  • Fixed scalar mapping to delegate to conversions to handle other primitive types with explicit discriminant. This handles the case of
#[derive(Deserialize, PartialEq, Debug)]
    enum E {
        A(String),
        B(String),
        C(f32), // <-- this is new
}

when a call is being made for de::VariantAccess::newtype_variant it delegate correctly to get the value, see enum Message type in test_de.rs

  • Added unit testing for use cases for passing happy case
  • Added unit testing into error case for corner use case

…sing for serde_yaml

- Co-change needed in yaml_rust to propogate Tags as a part of Sequence/Mapping Start events to map and use for deserialization
- Fixed scalar mapping to delegate to conversions to other primitive types when handling explicit discriminants, not just for '!!' case alone
- Added unit testing for use cases for passing happy case
- Added unit testing into error case for corner use case
Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

As of 0.9, serde-yaml no longer depends on yaml-rust, so I'll close this version of the implementation. Thanks anyway for the draft PR! Please take a look at 0.9 and if there is remaining work you still need, let's handle that in a separate PR.

@dtolnay dtolnay closed this Jul 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants