Skip to content

Conversation

@MabezDev
Copy link
Member

Adds ser/de for the following types:

  • unit types, i.e () & struct Test;
  • newtype structs, ser/de'ing the underlying value
  • Variants:
    • The newtype_variant format, i.e Variant(u32)
    • The struct_variant format, i.e Variant { x: u32, y: u16 }

ser:
    * Added newtype serialization (as underlying value)
    * Added newtype_variant serialization (as object)
    * Added unit serialization (as null)

der:
    * Added unit de
    * Added newtype_struct
* Implement VariantAccess for non unit enums
* Add newtype_variant & struct_variant deserialization
* Basic tests for both
* Remove Unreachable impl for SerializationStructVariant
@MabezDev MabezDev changed the title Newtypes variants and unit Newtype, variants and unit Feb 21, 2020
@MabezDev MabezDev requested a review from japaric February 21, 2020 12:13
@MabezDev MabezDev force-pushed the newtypes-variants-and-unit branch from 8ea4005 to 36cd0ce Compare February 21, 2020 14:47
@MathiasKoch
Copy link
Collaborator

+1 on this! Did a test yesterday, and it also fixes support for externally tagged enums (https://serde.rs/enum-representations.html)

Good job!

@MathiasKoch
Copy link
Collaborator

Ping @japaric

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Other than the merging with #36 (or #27?), looks good to me.
An entry to the changelog would also be necessary, but maybe we can add all those entries in a dedicated PR after merging all of the currently pending ones.


impl<'a, B> ser::SerializeStructVariant for SerializeStructVariant<'a, B>
where
B: ArrayLength<u8>,
Copy link
Member

Choose a reason for hiding this comment

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

This will need to be adapted once #36 lands

@eldruin
Copy link
Member

eldruin commented Dec 11, 2020

@MathiasKoch how do you want to go about this one?
Would you be able to pull these changes into a new PR and adapt them?

@MathiasKoch
Copy link
Collaborator

Sure thing, give me 5 minutes.

Will try to fix the CI clippy stuff at the same time.

MathiasKoch added a commit to MathiasKoch/serde-json-core that referenced this pull request Dec 11, 2020
@MathiasKoch
Copy link
Collaborator

@MabezDev Thank you very much for your work here!

I am closing this, as i have moved it to #42 with a rebase on master, to get this moving.

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