Transformer for RelativeTimeFormat component #2822
Conversation
| pub display_name: Option<Cow<'data, str>>, | ||
| /// Mapping for relative time fields. | ||
| #[cfg_attr(feature = "serde", serde(borrow))] | ||
| pub relatives: ZeroMap<'data, i8, str>, |
There was a problem hiding this comment.
Thought: What is the full set of number offsets? In most locales it is -1, 0, 1; there are some that go to -2, -1, 0, 1, 2. This is an okay use of ZeroMap, but if there is never going to be more than -2 to 2, we may want to consider expanding that into the struct itself.
There was a problem hiding this comment.
The offsets are from [-2, 2].
| /// Relative time patterns data struct. | ||
| #[icu_provider::data_struct( | ||
| StandardRelativeTimeV1Marker = "relativetime/standard@1", | ||
| NarrowRelativeTimeV1Marker = "relativetime/narrow@1", |
There was a problem hiding this comment.
Thought: I'm wondering whether we want to slice this smaller. For example, I know ECMA-402 delays the unit choice to the format function, but should we be making it an option in the constructor instead? And how about the direction (past/future)? We're going to get some pretty big data keys out of this, which begs the question about whether we should be slicing more granularly.
It's OK to make constructors that load multiple data keys for convenience, of course.
One possible axis that is already in ECMA-402 is "numeric". If the user requests numeric: "always", then they shouldn't need to load the non-numeric data. So, we could maybe move that data into a separate data key.
There was a problem hiding this comment.
The data keys are pretty big currently with most of the fields being 'None'.
I'm in favour of splitting it further into at least | "unit" x "width" | data keys. This would remove the need for having an array of "RelativeTimePattern".
Then we can focus on an efficient representation of "PluralRulesCategory" => "Singular substitution pattern" mapping.
Using a "ZeroMap<PluralRulesCategory, SubstitutionPattern>" would be good as we don't store the unnecessary mappings but I haven't been able to make it work.
There was a problem hiding this comment.
Would be great to do unit x width (8*3). What do you think about unit x width x numeric (8*3*2)?
Yes, let's brainstorm the best representation for PluralCategory => Pattern.
There are some advantages to expanding to separate fields instead of using ZeroMap. Note that None fields do not take up much space in Postcard.
pdogr
requested review from
a team,
robertbastian and
Manishearth
as code owners
pdogr
changed the title
|
Overall looks pretty great! |
- Remove "{0}" from substitution pattern string
- Clean code in cldr_serde/date_fields.rs
- Improve docs for relativetime/src/provider.rs
|
Updated the postcard data after merging. |
| relativetime/narrow/hour@1, en, 50B, 2a119b1d4dfa007dbc4d2e4d79cf39bd23b39167399b5878fea8cd6e7269fb5b | ||
| relativetime/narrow/hour@1, en-001, 48B, f15555b8ed2135cb792b46db2aa2b769423fc763fc8bb348ce58a7eff542ee43 | ||
| relativetime/narrow/hour@1, en-ZA, 48B, f15555b8ed2135cb792b46db2aa2b769423fc763fc8bb348ce58a7eff542ee43 |
There was a problem hiding this comment.
Praise: Note how for narrow hour, en differs from en-001. Since these are small keys, only the difference for narrow hour needs to be stored, and we can keep deduplicating the rest of the time scales that are identical. With a bigger key, a single change like this would "spoil" the deduplication from working most optimally.
|
This is great, thanks! |
No description provided.