Skip to content

Partial Rust Enum Serialization/Deserialization Support#60

Closed
jdeschenes wants to merge 11 commits intoflavray:masterfrom
jdeschenes:support_enum
Closed

Partial Rust Enum Serialization/Deserialization Support#60
jdeschenes wants to merge 11 commits intoflavray:masterfrom
jdeschenes:support_enum

Conversation

@jdeschenes
Copy link
Contributor

@jdeschenes jdeschenes commented Jan 4, 2019

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
@jdeschenes jdeschenes mentioned this pull request Jan 4, 2019
Open
@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 .

@poros poros mentioned this pull request Jan 14, 2020
Closed
@jdeschenes
Copy link
Contributor Author

jdeschenes commented Jan 16, 2020

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

poros
poros approved these changes Jan 23, 2020
View reviewed changes
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

calvinbrown085 commented Feb 26, 2020

@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

calvinbrown085 commented Feb 29, 2020

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

calvinbrown085 commented Mar 2, 2020

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

@calvinbrown085 calvinbrown085 mentioned this pull request Mar 2, 2020
Merged
@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

Reviewers

@poros poros poros approved these changes

@flavray flavray Awaiting requested review from flavray

Assignees

No one assigned

Labels

feature request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jdeschenes @0x003e @poros @calvinbrown085