Skip to content
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.

Sign up for GitHub

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 LongCurrencyFormatter for Long Currency Formatting #5351

Merged
merged 18 commits into from
Aug 26, 2024
Merged

Implement LongCurrencyFormatter for Long Currency Formatting #5351

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
0dbd034
Create LongCurrencyFormatter
younies Aug 9, 2024
852edb6
chore: Add `CurrencyPatternsDataV1` to `LongFormattedCurrency`
younies Aug 9, 2024
a40cf58
refactor: Update LongCurrencyFormatter constructor to accept currency…
younies Aug 9, 2024
071721b
refactor: Update LongCurrencyFormatter to include plural rules in con…
younies Aug 9, 2024
e9c607a
refactor: Add PluralCategory conversion for Count enum in extended_cu…
younies Aug 9, 2024
fcf7844
Fix and implement
younies Aug 9, 2024
a9e6ca9
refactor: Update currency formatting to include currency code in long…
younies Aug 9, 2024
58a180a
Fix doctest
younies Aug 9, 2024
9ac101e
refactor: Update currency formatting to include currency code as refe…
younies Aug 12, 2024
a6fc125
Revert "refactor: Update currency formatting to include currency code…
younies Aug 13, 2024
b08407e
refactor: Update LongFormattedCurrency to use interpolated pattern in…
younies Aug 13, 2024
695fd05
refactor: fix
younies Aug 13, 2024
993a4e4
refactor: add a todo and panic
younies Aug 15, 2024
e85e959
Add comment and fix a return error
younies Aug 15, 2024
f57137b
Merge branch 'main' into long-currency-formatter
younies Aug 15, 2024
9642f32
Merge remote-tracking branch 'upstream' into long-currency-formatter
younies Aug 22, 2024
562be18
Fix comments
younies Aug 22, 2024
05e805e
Merge branch 'main' into long-currency-formatter
younies Aug 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion components/experimental/src/dimension/currency/long_format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ impl<'l> Writeable for LongFormattedCurrency<'l> {
let pattern = patterns
.get(&pattern_count)
.or_else(|| patterns.get(&PatternCount::Other))
.ok_or(core::fmt::Error)?;
// According to [Unicode Technical Standard #35](https://unicode.org/reports/tr35/tr35-numbers.html),
// The `other` pattern must be present in the data.
// Also, if the `other` pattern is not present, the datagen will fail.
.unwrap();

// TODO: Remove this line once the pattern is already implemented in the provider.
// Parse the pattern string into a DoublePlaceholderPattern
Expand Down
12 changes: 3 additions & 9 deletions provider/data/experimental/data/currency_patterns_data_v1_marker.rs.data
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 2 additions & 3 deletions provider/data/experimental/fingerprints.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29347,15 +29347,14 @@ currency/extended@1, zu/YER, 68B, 52c541c97403eaa6
currency/extended@1, zu/ZAR, 75B, 854159a2eeed4880
currency/extended@1, zu/ZMK, 85B, f9b17cba085ca192
currency/extended@1, zu/ZMW, 71B, 608ab48db3a48aad
currency/patterns@1, <lookup>, 94B, 14 identifiers
currency/patterns@1, <total>, 617B, 9 unique payloads
currency/patterns@1, <lookup>, 91B, 13 identifiers
currency/patterns@1, <total>, 569B, 8 unique payloads
currency/patterns@1, blo, 82B, da128faf13fad0d5
currency/patterns@1, ceb, 72B, db7c7c84fecaf3d0
currency/patterns@1, es-GT, 72B, -> ceb
currency/patterns@1, ja, 61B, 9af35aad6ebc7d70
currency/patterns@1, my, 62B, 86cee97085536d8b
currency/patterns@1, ro, 85B, 629b2b7dcea97875
currency/patterns@1, sd, 48B, 3a6d9523170345d0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems to be a CLDR bug (missing Other), can you file an issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have already filed this issue: #5374. I want to discuss it with @sffc today to ensure that it is a CLDR issue before filing it. If it is a CLDR issue, we need to decide what to do in the meantime. We can use the default {0} {1} or the pattern from en-US.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

currency/patterns@1, si, 71B, 4f661a4fd0c9c312
currency/patterns@1, sw, 72B, -> ceb
currency/patterns@1, to, 62B, -> my
Expand Down
6 changes: 6 additions & 0 deletions provider/source/src/currency/patterns.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@ impl DataProvider<CurrencyPatternsDataV1Marker> for SourceDataProvider {
.get(default_system)
.ok_or(DataErrorKind::IdentifierNotFound.into_error())?;

// According to [Unicode Technical Standard #35](https://unicode.org/reports/tr35/tr35-numbers.html),
// The `other` pattern must be present in the data.
if patterns.pattern_other.is_none() {
return Err(DataErrorKind::IdentifierNotFound.into_error());
}

Ok(DataResponse {
metadata: Default::default(),
payload: DataPayload::from_owned(CurrencyPatternsDataV1 {
Expand Down