-
Notifications
You must be signed in to change notification settings - Fork 10
Add serde(skip) attribute to PhantomData marker fields
#202
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The relevant impl on the serde side is:
https://docs.rs/serde/1.0.228/src/serde/core/ser/impls.rs.html#117-128
That is, the serialised form of the cs field will never actually provide any "type safety", so this change is very low risk.
There are reasonable arguments for wanting to express the colour space in the serialised form. That adds extra type safety, but could easily cause confusion (e.g. if someone tries to change the color space and instead gets a crash). But the UX for this serialisation won't be great; it will just be opaque numbers (pun not intended). That isn't an issue in this PR, but if we were to make breaking changes here, we should probably batch them with this change.
These impls were added in #61; @waywardmonkeys can you speak to whether the "type safety" was ever reasoned about here?
This PR looks good, other than the one changelog nit. Once that's done, I'll be happy to merge this. Thanks!
CHANGELOG.md
Outdated
|
|
||
| ### Changed | ||
|
|
||
| * Skip serializing/deserializing `PhantomData` marker fields of `OpaqueColor`, `AlphaColor`, and `PremulColor` when the `serde` feature is enabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs the PR and author links (don't forget to also add these to the link reference definitions at the bottom)
|
serde was only added because it was needed by peniko ... I think I wish that we hadn't added it and just bit the bullet on removing it from Peniko. I don't know if I feel comfortable removing this ... without it, you can't know what was serialized. But then, you don't really know anyway from it, I guess. Thoughts, @tomcur ? @alvinisspicy What're you using serde + color for? |
|
Thanks for the feedback and context! @waywardmonkeys, I am not sure if you were referring specifically to adding
On the question of “type safety” from the
This PR does not cause any breaking changes. By default, serde will skip any additional unknown fields during deserialization (https://serde.rs/container-attrs.html#deny_unknown_fields). If, in the future, there is interest in incorporating the color space into the serialized format in a meaningful way (e.g., a tagged enum or string name), that should be introduced in a separate, explicitly breaking change with a clearer UX around how color spaces are encoded. For additional context, I encountered this issue while trying to deserialize |
68221c3 to
5e51403
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to skip these fields. This will be a breaking change (e.g. deserializing using the old version will expect the cs field to be present, throwing an error otherwise).
|
Thanks! |
What does this PR do?
#[cfg_attr(feature = "serde", serde(skip))]to thePhantomDatamarker fields inOpaqueColor,AlphaColor, andPremulColor.Why is this needed?
csmarker purely at the type level and out of the serialized representation.tomlcrate) to serialize and deserialize these color types.Behavioral changes:
PhantomDatais zero-sized and was not carrying data.csfield (if present) is now ignored.