Impl Serde for PackedPatternsV1; export Serde impls for PluralElements #5592
Conversation
sffc
requested review from
Manishearth,
zbraniecki and
nordzilla
as code owners
sffc
requested review from
robertbastian
and removed request for
zbraniecki and
nordzilla
|
🎉 All dependencies have been resolved ! |
|
@Manishearth Do you prefer the "human-readable" serialized format here, which requires shipping the builder code (which is not bad but not great), or would you prefer that I serialize something closer to the binary format (the header int and a sequence of patterns) such that I don't need to ship the builder? |
|
Hmm, so I don't actually recall if llvm is smart enough to eliminate human readable deserialization code when working with postcard. My concern is codesize bloat for postcard users. Otherwise having it be actually human readable seems like a good choice. I don't have a strong preference for it. |
| @@ -31,12 +33,20 @@ pub struct LengthPluralElements<T> { | |||
|
|
|||
| /// A builder for a [`PackedPatternsV1`]. | |||
| #[derive(Debug, Clone, PartialEq, Eq)] | |||
| #[cfg_attr(feature = "serde", derive(serde::Deserialize))] // human-readable only | |||
| #[cfg_attr(feature = "datagen", derive(serde::Serialize))] // human-readable only | |||
There was a problem hiding this comment.
I don't think a builder type should implement serde.
| @@ -20,6 +20,8 @@ use crate::pattern::runtime::Pattern; | |||
|
|
|||
| /// A field of [`PackedPatternsBuilder`]. | |||
| #[derive(Debug, Clone, PartialEq, Eq)] | |||
| #[cfg_attr(feature = "serde", derive(serde::Deserialize))] // human-readable only | |||
| #[cfg_attr(feature = "datagen", derive(serde::Serialize))] // human-readable only | |||
There was a problem hiding this comment.
You say this, but there's nothing stopping anyone from using this directly in a data struct, or with a non-human-readable format.
There was a problem hiding this comment.
It is a type exported in a provider module.
There was a problem hiding this comment.
That doesn't stop anyone from using it in a data struct
There was a problem hiding this comment.
I'm closing this PR based on the other comments, but I don't think I agree with the premise "That doesn't stop anyone from using it in a data struct": I think it is our duty to help prevent common bugs in clients of ICU4X, but it seems wrong to restrict ICU4X development for the sake of preventing ICU4X developers from doing that. We have a high standard for code with thorough review processes and CI in place.
| pub standard: LengthPluralElements<Pattern<'a>>, | ||
| /// Patterns for variant 0. If `None`, falls back to standard. | ||
| #[cfg_attr(feature = "serde", serde(borrow))] | ||
| #[cfg_attr(feature = "serde", serde(skip_serializing_if = "Option::is_none"))] |
There was a problem hiding this comment.
This only works for human-readable formats, which is not enforced anywhere.
I'm pretty sure it does; the deserialize impl is generic in |
|
@robertbastian Your comments don't make a lot of sense to me. I can be convinced of avoiding Serde impls on types exported from a library. I don't see the argument for adding to code complexity for Serde impls on types that live in the provider module. |
| /// | ||
| /// # Serialization | ||
| /// | ||
| /// Although this type implements Serde traits, ICU4X developers and clients | ||
| /// seeking a more efficient bitpacked representation should consider | ||
| /// [`crate::provider::PluralElementsPackedCow`]. | ||
| pub struct PluralElements<T>(PluralElementsInner<T>); |
There was a problem hiding this comment.
@robertbastian I added you to the review primarily to give you an opportunity to raise an objection to this change, in icu_plurals (I noted this in the OP). Given the lack of comment in this file, is it safe to assume that you are okay with this change?
There was a problem hiding this comment.
Thanks for pointing this out, I do have objections. The serde impls on PluralElementsInner are currently only used in the human readable code path; this change uses them for all serializers through PluralElements. skip_serializing_if does not work for non-self-describing formats like postcard.
|
Requesting re-review for clarification of the nature of the comments. |
|
OK the approach I put forth in this PR is not going to be compatible with @robertbastian's comments so I will close this PR and make a new one that uses "something closer to the binary format". |
Depends on #5580
I know @robertbastian previously was not a fan of exporting a Serde impl for
PluralElements. I can work around this by movingPluralElementsInnerto theicu_plurals::providermodule. However, I think it is fine for crates to export Serde impls for their core types. We do this in various other places. The main difference/downside in this particular case is that we have a bitpacked representation in the provider module that most ICU4X devs should be using; I am using thePluralElementsimpl only for human-readable serialization.