Conversation
RickyDaMa
left a comment
RickyDaMa
left a comment
There was a problem hiding this comment.
Glad to see you're enjoying your time off! Gave this a quick once-over for you :)
| /// Describes a single axis subset for a variable font. | ||
| #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] | ||
| #[serde(untagged)] | ||
| pub enum AxisSubset { | ||
| /// Describes a range of an axis. | ||
| Range { | ||
| /// The name of the axis under consideration. | ||
| name: String, | ||
| /// Optionally, the lower end of the range, in user coordinates | ||
| #[serde(rename = "@userminimum", default, skip_serializing_if = "Option::is_none")] | ||
| user_minimum: Option<f64>, | ||
| /// Optionally, the upper end of the range, in user coordinates | ||
| #[serde(rename = "@usermaximum", default, skip_serializing_if = "Option::is_none")] | ||
| user_maximum: Option<f64>, | ||
| /// Optionally, the new default value of subset axis, in user coordinates. | ||
| #[serde(rename = "@userdefault", default, skip_serializing_if = "Option::is_none")] | ||
| user_default: Option<f64>, | ||
| }, | ||
| /// Describes a single point of an axis. | ||
| Discrete { | ||
| /// The name of the axis under consideration. | ||
| name: String, | ||
| /// The single point of the axis. | ||
| #[serde(rename = "@uservalue")] | ||
| user_value: f64, | ||
| }, | ||
| } |
There was a problem hiding this comment.
It might be more useful/helpful to use inner structs for the range/discrete values here. By keeping the values in the enum itself, you have to hold an AxisSubset and can only access inner fields with matching, whereas if there's an inner struct you can pull (a reference to) that out the first time you match and hold that for future use.
So, unless you specifically expect users to only need to match on this enum and inspect its values once, I'd recommend creating AxisSubsetRange and AxisSubsetDiscrete structs (or similar)
There was a problem hiding this comment.
By this you mean going for an enum AxisSubset { Range(AxisSubsetRange), Discrete(AxisSubsetDiscrete) }?
There was a problem hiding this comment.
Yes, sorry if that wasn't clear!
There was a problem hiding this comment.
Both with the current and your suggested approach, I run into DeError(Custom("data did not match any variant of untagged enum AxisSubset")) when trying to load MutatorSans2.designspace 🤔 Not sure what's going on there...
There was a problem hiding this comment.
Do the name fields need a #[serde(rename = "@name")] annotation?
There was a problem hiding this comment.
Oh DUH, thanks. Now I get a different error. Progress.
There was a problem hiding this comment.
When I move the structs out, I am back at the "data did not match any variant" error and when I leave them as they are, the discrete variant is deserialized as a ranged one. The joys of untagged data.
There was a problem hiding this comment.
Put the discrete variant first in the enum declaration, as it's harder to match (requiring name and user_value). If serde isn't rejecting unknown fields, then discrete can match range.
As for why decomposing the enum isn't working, it probably something to do with how (IIRC) the XML deserialisation cares about the struct name(?) Worth finding a workaround to before we merge this though imo
There was a problem hiding this comment.
Moving the variant up top also didn't work... Maybe I'll need to write a custom function for this.
2 participants
Todo: