feat(currency): Migrate compact currency formatting to generic design and simplify options#8182
Closed
younies wants to merge 1 commit into
Closed
feat(currency): Migrate compact currency formatting to generic design and simplify options#8182younies wants to merge 1 commit into
younies wants to merge 1 commit into
Conversation
younies
force-pushed
the
compact-currency-formatting
branch
2 times, most recently
from
561ca8b to
f077a3d
Compare
younies
commented
Jul 7, 2026
| pub struct CompactData { | ||
| // TODO(#8184): Currently unused in format_fixed_decimal while waiting for CLDR/TR35 investigation | ||
| // regarding compact currency pattern gluing vs explicit double-placeholder data. | ||
| pub(crate) _short_currency_compact: DataPayload<ShortCurrencyCompactV1>, |
Member
Author
There was a problem hiding this comment.
remove this / or comment it as we are not going to use it for now, and add a TODO(#8184) at the ShortCurrencyCompactV1 struct to state that it maybe removed if the CLDR committee decided to use the currency pattern directly
younies
commented
Jul 7, 2026
|
|
||
| plural_rules: PluralRules, | ||
| /// Type alias for compact currency formatting. | ||
| pub type CompactCurrencyFormatter = CurrencyFormatter<Compact>; |
Member
Author
There was a problem hiding this comment.
remove this type alias, we are going to do this before the graduation
Member
Author
There was a problem hiding this comment.
Done! Removed CompactCurrencyFormatterPreferences and CompactCurrencyFormatter type aliases.
younies
commented
Jul 7, 2026
| where | ||
| D: ?Sized | ||
| + DataProvider<CurrencyEssentialsV1> | ||
| + DataProvider<ShortCurrencyCompactV1> |
Member
Author
There was a problem hiding this comment.
we do not need this for now : ShortCurrencyCompactV1
Member
Author
There was a problem hiding this comment.
Done! Removed ShortCurrencyCompactV1 data requirement from CompactData, constructors, and trait bounds.
… and simplify options This commit modernizes compact currency formatting in ICU4X by: 1. Architecture Migration: Migrating CompactCurrencyFormatter to adopt the generic CurrencyFormatter<Compact> design with try_new_short and try_new_narrow constructors (where currency is bound at construction time). 2. Options Simplification: Removing CurrencyFormatterOptions from options.rs and replacing it with a direct pub(crate) width: Width field across CurrencyFormatter<V>, as width is now determined at construction time. 3. Pattern Gluing: In format_fixed_decimal, we use name_and_pattern from CurrencyEssentialsV1 (which resolves standard vs standard_alpha_next_to_number patterns), format the number via compact decimal (compact_data), and glue the compact number with the currency symbol. TAG=agy CONV=c033380d-144b-4f6b-b091-b5c89c4f80a1
younies
force-pushed
the
compact-currency-formatting
branch
from
f077a3d to
b853de3
Compare
younies
requested review from
robertbastian and
sffc
and removed request for
robertbastian
| pub struct Compact; | ||
|
|
||
| impl ValueRepresentation for Compact { | ||
| type Data = super::compact_formatter::CompactData; |
Member
There was a problem hiding this comment.
question: why isn't this CompactDecimalFormatter? then Decimal::Data can be DecimalFormatter
robertbastian
added a commit
that referenced
this pull request
Jul 8, 2026
…rmatter<V> using AbstractFormatter (#8189) ## Summary This PR merges all remaining currency formatters into a single unified `CurrencyFormatter<V>` type by leveraging `AbstractFormatter`, proposing an alternative architecture to #8182. Instead of maintaining separate representation structs or duplicating formatting data (such as `decimal_formatter`, `compact_data`, and `plural_rules`), `CurrencyFormatter<V>` directly wraps an underlying value formatter (`DecimalFormatter` for standard currency formatting or `CompactDecimalFormatter` for compact currency formatting). This unifies the currency formatting architecture, simplifies pattern interpolation, and significantly reduces boilerplate across the module. ## Changelog - **Architecture Unification**: Refactored `CurrencyFormatter<V>` to constrain `V: AbstractFormatter`, allowing it to wrap either `DecimalFormatter` or `CompactDecimalFormatter` directly in a single `value_formatter: V` field. - **Constructor Streamlining**: - Standard constructors (`try_new_short`, `try_new_narrow`, `try_new_long`) initialize `CurrencyFormatter` with `DecimalFormatter`. - Compact constructors (`try_new_compact_short`, `try_new_compact_narrow`, `try_new_compact_long`) initialize `CurrencyFormatter` with `CompactDecimalFormatter`. - **`icu_decimal` Delegation**: Exposed `pub(crate)` helper methods (`format_unsigned`, `format_sign`) and exported `FormattedUnsignedCompactDecimal` / `AbstractFormatter` from `icu_decimal` so currency formatters can delegate numeric formatting directly to the underlying value formatter. - **Boilerplate & Code Reduction**: Removed redundant data fields and simplified formatting logic across `compact_formatter.rs` and `formatter.rs`, resulting in a net code reduction of over 200 lines (+665 additions, -876 deletions). --------- Co-authored-by: Younies Mahmoud <younies.mahmoud@gmail.com>
Member
Author
|
Closing in favor of #8189, which unifies all currency formatters under AbstractFormatter. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR modernizes compact currency formatting in ICU4X by adopting the unified generic
CurrencyFormatter<V>architecture, simplifying options across all currency formatters, and establishing the foundational pattern-gluing logic for compact numbers.Changelog
Architecture Migration:
CompactCurrencyFormatterto adopt the genericCurrencyFormatter<Compact>design withtry_new_shortandtry_new_narrowconstructors (binding currency at construction time, mirroring PR feat(currency): Migrate CurrencyFormatter to adopt the new design with generic ValueRepresentation (Decimal) and add short/narrow constructors #8145 forDecimal).load_with_fallback.CompactCurrencyFormatterandCompactCurrencyFormatterPreferences).Options Simplification:
CurrencyFormatterOptionsfromoptions.rs, leaving onlyWidth(Short/Narrow).options: CurrencyFormatterOptionswith a directpub(crate) width: Widthfield acrossCurrencyFormatter<V>, as width is now determined at construction time.Pattern Gluing:
format_fixed_decimal, we usename_and_patternfromCurrencyEssentialsV1(which resolvesstandardvsstandard_alpha_next_to_numberpatterns based on whether the symbol is alphabetical).compact_data) and glues the resulting compact number with the currency symbol.format_fixed_decimalfor implementingShortCurrencyCompactV1pattern lookup in a follow-up PR.Testing
icu_experimentalcompile and pass cleanly.