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

Partial Rust Enum Serialization/Deserialization Support #60

Closed
wants to merge 11 commits into from

Conversation

jdeschenes
Copy link
Contributor

Hello,

Currently, it is not possible to have enums beyond the unit in this library. This is an attempt to support enums.

Let me know if there are adjustments would you like to be done.

Implementation

There are four types of enums:

enum Unit {
    Imperial,
    Metric,
}

enum Value {
    Bool(bool),
    Float(f32),
}

enum Tuples {
    Coord2(f32, f32)
    Mixed3(f32, f32, i32)
}

enum Structs {
    Coord2 { x: f32, y: f32 }
    Coord3 { x: f32, y: f32 }
}

Here are the conversions that will be applied:

Unit

{ "type": "enum", "name": "Unit", "symbols": ["Imperial", "Metric"] }

Value

{ "name": "some_value", "type": {
    "type": "record",
    "name": "Value",
    "fields": [
        { "name": "type", "type": "enum", "symbols": ["Bool", "Float"] },
        { "name": "value", "type": ["boolean", "float"] }
    ]}
} 

Tuples

{ "name": "some_tuple", "type": {
    "type": "record",
    "name": "Tuples",
    "fields": [
        { "name": "type", "type": "enum", "symbols": ["Coord2", "Mixed3"] },
        { "name": "value", "type": [
            { "type": "array", "items": ["float"] },
            { "type": "array", "items": ["int", "float"] }
        ]}        
    ]
}

Structs

{ "name": "some_struct", "type": {
    "type": "record",
    "name": "Structs",
    "fields": [
        {"name": "type", "type": "enum", "symbols": ["Coord2", "Coord3"] },
        {"name": "value", "type": [
            { "name": "Coord2", "fields": [
                { "name": "x", "type": "float" },
                { "name": "y", "type": "float" }
            ]},
            { "name": "Coord3", "fields": [
                { "name": "x", "type": "float" },
                { "name": "y", "type": "float" },
                { "name": "z", "type": "float" }
            ]}
        ]}
    ]}
}

Some additional information

  1. I have taken the liberty of adding some unit tests in order to make sure that the implementation was
    respected.
  2. I removed the mut from the trait implementation of the Deserializer, it was not needed anywhere.
    Let me know if there is a need for it.

Limitations

  1. Internal, adjacent and untagged enums are not supported. I did not figure out how to create those.
    I kept the serialization variants, but they are bogus. The Unit enums still works but they are
    serialized as string.
  2. The "type" and "value" field names are hardcoded. Without fixing point 1, I don't know how
    it should be done.
  3. The library does not yet support schemas with more than one named type within a single union.
    So it is not feasible to create a schema of the "Tuples" and "Structs" example as they are.

@jdeschenes jdeschenes changed the title Partial Enum Support Partial Rust Enum Serialization/Deserialization Support Jan 4, 2019
@flavray flavray self-requested a review January 7, 2019 10:25
@0x003e
Copy link

0x003e commented Jan 12, 2020

Hi everyone,

I tested this feature and it's work perfectly, thanks @jdeschenes .
This feature can be merged ?

@poros
Copy link
Collaborator

poros commented Jan 14, 2020

Apologies, we kinda dropped on the ball on this one. Unfortunately the serde integration is the part of the code base I know the least, so it will take me a bit of time to review the code, but I'll look into it when I have a bit of free time to dedicate to this project.

The conflicts seem to be only style-related, so those should not be a blocker.

@poros poros self-requested a review January 14, 2020 21:19
@poros
Copy link
Collaborator

poros commented Jan 14, 2020

There is also #76 implementing bits and pieces of the same feature in a slightly different way and they are both pretty old... (definitely because of the fault of us maintainers).

I'll need to budget some time to properly understand how serde handles enums (tonight I only managed to read the serialization half of the data format) and at the same time follow up with the authors to understand who is still interested in contributing.

@jdeschenes are you still interested in contributing? Shall we try to revitalize this PR? I'll ask the same to @RGafiyatullin on #76 .

@jdeschenes
Copy link
Contributor Author

I don't mind taking a look. What do you want to be done?

Copy link
Collaborator

@poros poros left a comment

Choose a reason for hiding this comment

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

@jdeschenes it's great to know that you are still interested! :)

Sorry for the late reply, but I struggled to find the time to learn how to implement a serde deserializer. Anyway, now I finally managed to understand this PR and it seems pretty legit to me.

If you can fix the conflicts (they should just be a couple commas), I will proceed with merging it.

@poros
Copy link
Collaborator

poros commented Feb 13, 2020

@jdeschenes any problem with the merge conflicts I can help with?

@calvinbrown085
Copy link
Contributor

@jdeschenes @poros I’d love to help get this across the line. What else needs done here? Just fix the conflicts up?

@poros
Copy link
Collaborator

poros commented Feb 29, 2020

@calvinbrown085 yes, just fixing the merge conflicts

@calvinbrown085
Copy link
Contributor

I don't think I have permission to push to this fork :(

@poros
Copy link
Collaborator

poros commented Feb 29, 2020 via email

@calvinbrown085
Copy link
Contributor

Yeah, that does! I'll see if I can give that a whirl.

@poros
Copy link
Collaborator

poros commented Apr 15, 2020

Merged as part of #110

@poros poros closed this Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants