-
Notifications
You must be signed in to change notification settings - Fork 160
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
Implement Extended Currency DataModel #4706
base: main
Are you sure you want to change the base?
Conversation
a7c1b5a
to
6cd6f90
Compare
/// A mapping from each currency's ISO code to its associated formatting patterns. | ||
// Using CurrencyPatternConfig until implementing AsULE for ExtendedCurrencyPatternConfig. | ||
// use short for long right now. | ||
#[cfg_attr(feature = "serde", serde(borrow))] | ||
pub patterns_config: ZeroMap<'data, Count, CurrencyPatternConfig>, |
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.
Suggestion: I think in general the best way to store things that vary by plural form is to pull out the other
variant and store all the others in the map. Something more like
/// For plural form 'other'
pub pattern_config_other: CurrencyPatternConfig,
/// For all other plural forms and explicit forms
pub pattern_config_plurals: ZeroMap<'data, Count, CurrencyPatternConfig>,
This is good because
- The fallback to
other
is infallible - In locales that don't use plurals (like CJK), the ZeroMap becomes empty, which is nice
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.
great idea, we treated it as default pattern.
@sffc and @robertbastian when I run I received the following error message
|
It means that |
It looks like you forgot to add |
@sffc : thanks for the notice, but even I applied this, still I get the same message
|
Serde is too limited to borrow through an |
serde-rs/serde#2016 again |
Wow they locked that thread. |
Since the Serde issue doesn't keep track of linked issues any more, let's start using #1556 to keep track of all the cases where we've wasted time re-discovering and re-working-around this issue (which I still hold to be fully in scope for Serde core) |
right now, I added those fields to the display_names map in order to avoid this :) actually, this will reduce the datasize by replacing the none case with empty. |
This looks good, but I would like to see a way to reduce the number of currency JSON files generated in testdata if possible? Thoughts @robertbastian? |
No description provided.