-
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.
// } | ||
// } | ||
|
||
impl IterableDataProviderInternal<CurrencyExtendedDataV1Marker> for crate::DatagenProvider { |
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.
needs to generate the data locale with auxiliary key.
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.
fn neo_time_skeleton_supported_locales(&self) -> Result<HashSet<DataLocale>, DataError> { |
} | ||
|
||
impl IterableDataProvider<CurrencyExtendedDataV1Marker> for DatagenProvider { | ||
fn supported_requests(&self) -> Result<HashSet<(DataLocale, DataKeyAttributes)>, DataError> { |
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 have two questions:
- Shall we return the
HashSet
with theDataLocale
andDataKeyAttributes
without adding the auxiliary key to theDataLocale
? - How to create a
DataKeyAttributes
from aTinyAsciiStr<3>
which is the currency code ?
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.
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.
DataKeyAttributes
are not in theDataLocale
anymore. If you previously usedset_aux
, now just return theDataKeyAttributes
separatelyDataKeyAttributes::from_tinystr(x.resize())
No description provided.