localcc / gvas

GVAS file format parsing library for rust
MIT License
18 stars 4 forks source link

[serde] Property enum wrappers are excessively verbose when serialized #66

Closed scottanderson closed 4 months ago

scottanderson commented 5 months ago

The ArrayProperty and MapProperty types have redundant property information wrapped around every value in the container. However, ArrayProperties only support one property type at a time, and the same can be said for MapProperty keys or MapProperty values. It is unnecessary and distracting to display this information in serde form.

I created #52 as a proof of concept to show what an optimized structure might look like, but the PR accomplishes this by writing custom ser+deserialization logic. Due to the nature of MapProperty, it is not practical to write code for all of the <K, V> property type combinations, so the approach in that PR will not scale well. I am currently thinking the best option may be to convert MapProperty to an enum, where the variants have various concrete property <K, V> types, resulting in a cleaner serde-serialized form. For example, this might look like:

pub enum MapProperty {
    /// Map<EnumProperty, BoolProperty>
    EnumBool(IndexMap<String, bool>),
    /// Map<EnumProperty, IntProperty>
    EnumInt(IndexMap<String, i32>),
    /// Map<EnumProperty, Property>
    EnumProperty {
        value_type: String,
        value: IndexMap<String, Property>,
    },
    /// Map<NameProperty, BoolProperty>
    NameBool(IndexMap<String, bool>),
    /// Map<NameProperty, IntProperty>
    NameInt(IndexMap<String, i32>),
    /// Map<NameProperty, Property>
    NameProperty {
        value_type: String,
        value: IndexMap<String, Property>,
    },
    /// Map<Property, Property>
    PropertyProperty {
        key_type: String,
        value_type: String,
        value: IndexMap<Property, Property>,
    },
    /// Map<StrProperty, BoolProperty>
    StrBool(IndexMap<String, bool>),
    /// Map<StrProperty, IntProperty>
    StrInt(IndexMap<String, i32>),
    /// Map<StrProperty, Property>
    StrProperty {
        value_type: String,
        value: IndexMap<String, Property>,
    },
    /// Map<StrProperty, StrProperty>
    StrStr(IndexMap<String, String>),
}