currency: Implement runtime fallback for missing patterns and remove constructor validation#8066
currency: Implement runtime fallback for missing patterns and remove constructor validation#8066younies wants to merge 8 commits into
Conversation
…ternIndices Make standard_alpha_next_to_number, accounting_positive, and accounting_alpha_next_to_number_positive optional (Option<u8>) in PatternIndices to support true runtime fallback and avoid datagen-time fallback duplication. Also updated getters in CurrencyEssentials to implement the fallback hierarchies at runtime and added documentation comments for them. Regenerated experimental test data. TAG=agy CONV=7de8b65e-8ff1-43aa-9dbd-1a73ee7b45ff
| debug_assert!( | ||
| (self.indices.standard as usize) < self.patterns.len(), | ||
| "Standard pattern index {} is out of bounds for patterns of length {}", | ||
| self.indices.standard, | ||
| self.patterns.len() | ||
| ); |
There was a problem hiding this comment.
There was a problem hiding this comment.
please see my comment on the other PR
add by @younies #8050 (comment)
robertbastian
left a comment
There was a problem hiding this comment.
no, for several reasons:
- an
Option<u8>takes more space than au8 - it shifts work that can be done at datagen time to runtime
…alidation Revert the changes that made positive patterns optional in PatternIndices. Positive patterns are now u8 (non-optional) again, and fallback is resolved at datagen time. Added runtime fallback to FALLBACK_PATTERN in CurrencyEssentials getters for custom data support. Removed standard pattern validation from CurrencyFormatter and CompactCurrencyFormatter constructors, allowing them to succeed and use runtime fallback if the pattern is missing. Regenerated experimental baked data. TAG=agy CONV=2329ca53-2c21-4bb8-bc21-4b69db92e9cc
- Remove `.unwrap()` from positive pattern getters in `icu_provider_source` tests, as positive patterns are now non-optional. - Add `#[cfg(feature = "chrono")]` to `test_against_chrono` in `zoneinfo64` to fix compilation failure when `chrono` feature is not enabled. TAG=agy CONV=2329ca53-2c21-4bb8-bc21-4b69db92e9cc
|
I have made all pattern indices Regarding the struct size, we will address that when we add all the patterns and implement the |
| // we have the fallback here for users feeding their own data without | ||
| // handling the fallback logic in their data generation. |
There was a problem hiding this comment.
You should document the invariant that the standard pattern always exists, and phrase any case where it doesn't as a condition that we don't support.
| // we have the fallback here for users feeding their own data without | |
| // handling the fallback logic in their data generation. | |
| // we have the fallback here in case the data struct is corrupted. |
| debug_assert!( | ||
| (self.indices.standard as usize) < self.patterns.len(), | ||
| "Standard pattern index {} is out of bounds for patterns of length {}", | ||
| self.indices.standard, | ||
| self.patterns.len() | ||
| ); | ||
| self.patterns | ||
| .get(self.indices.standard as usize) | ||
| .or_else(|| self.patterns.get(0)) | ||
| .unwrap_or(FALLBACK_PATTERN) |
There was a problem hiding this comment.
| debug_assert!( | |
| (self.indices.standard as usize) < self.patterns.len(), | |
| "Standard pattern index {} is out of bounds for patterns of length {}", | |
| self.indices.standard, | |
| self.patterns.len() | |
| ); | |
| self.patterns | |
| .get(self.indices.standard as usize) | |
| .or_else(|| self.patterns.get(0)) | |
| .unwrap_or(FALLBACK_PATTERN) | |
| self.patterns | |
| .get(self.indices.standard as usize) | |
| .unwrap_or_else(|| { | |
| debug_assert!( | |
| false, | |
| "Standard pattern index {} is out of bounds for patterns of length {}", | |
| self.indices.standard, | |
| self.patterns.len() | |
| ); | |
| // GIGO | |
| FALLBACK_PATTERN | |
| }) |
| } | ||
|
|
||
| #[test] | ||
| #[cfg(feature = "chrono")] |
There was a problem hiding this comment.
don't make unrelated changes.
| /// Even though the baked data handles the fallback at data generation time, | ||
| /// we have the fallback here for users feeding their own data without | ||
| /// handling the fallback logic in their data generation. |
There was a problem hiding this comment.
there is no fallback here
| self.patterns | ||
| .get(self.indices.standard_alpha_next_to_number as usize) | ||
| .or_else(|| self.standard_pattern()) | ||
| .unwrap_or_else(|| self.standard_pattern()) |
There was a problem hiding this comment.
| .unwrap_or_else(|| self.standard_pattern()) | |
| .unwrap_or_else(|| { | |
| debug_assert!( | |
| false, | |
| "Standard-alpha-next-to-number pattern index {} is out of bounds for patterns of length {}", | |
| self.indices.standard, | |
| self.patterns.len() | |
| ); | |
| // GIGO | |
| FALLBACK_PATTERN | |
| }) |
| /// Fallback hierarchy: | ||
| /// `standard_alpha_next_to_number` -> `standard` | ||
| /// | ||
| /// Even though the baked data handles the fallback at data generation time, | ||
| /// we have the fallback here for users feeding their own data without | ||
| /// handling the fallback logic in their data generation. |
There was a problem hiding this comment.
don't document like this. we should treat the fallback as a datagen concern, here we just assume we have a value for each type
| /// Fallback hierarchy: | |
| /// `standard_alpha_next_to_number` -> `standard` | |
| /// | |
| /// Even though the baked data handles the fallback at data generation time, | |
| /// we have the fallback here for users feeding their own data without | |
| /// handling the fallback logic in their data generation. |
|
|
||
| /// Returns the `standard_alpha_next_to_number` negative pattern if specified, falling back to standard negative. | ||
| /// | ||
| /// Fallback hierarchy: |
There was a problem hiding this comment.
issue: handle the negative fallbacks at datagen time as well. they can still be Option<u8>
This PR implements runtime fallback for currency patterns in
CurrencyEssentialsand relaxes validation in formatters to support custom data that may lack some patterns.Changelog
CurrencyEssentialsnow return non-optional&DoublePlaceholderPattern. If a pattern index is out of bounds (representing a missing pattern in custom data), they fall back according to the hierarchy, withFALLBACK_PATTERN("{1}{0}") as the ultimate fallback.standard_alpha_next_to_number_positive➔standardaccounting_positive➔standardaccounting_alpha_next_to_number_positive➔standard_alpha_next_to_number➔standardOption<&DoublePlaceholderPattern>, withaccounting_negativefalling back tostandard_negativeif missing (None).CurrencyFormatterandCompactCurrencyFormatterconstructors. They now initialize successfully even if the standard pattern is missing in the data, relying on runtime fallback.