Restructure Intl implementation to use ICU4X's locale preferences#4589
Restructure Intl implementation to use ICU4X's locale preferences#4589jedel1043 wants to merge 6 commits intoboa-dev:mainfrom
Conversation
jedel1043
added
Internal
jedel1043
force-pushed
the
icu4x-prefs
branch
from
c1c339c to
796ca0b
Compare
Test262 conformance changes
Fixed tests (1): |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4589 +/- ##
==========================================
+ Coverage 47.24% 56.50% +9.26%
==========================================
Files 476 547 +71
Lines 46892 60036 +13144
==========================================
+ Hits 22154 33926 +11772
- Misses 24738 26110 +1372 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| result | ||
| } | ||
|
|
||
| fn intersection(&self, other: &Self) -> Self { |
There was a problem hiding this comment.
Preferences have a merge operation already
There was a problem hiding this comment.
This the opposite though; it takes two preference objects and gets the preferences that are the same in both.
We need this because ECMA402 requires correctly setting up the locale with the extensions that were "used" by the locale resolution algorithm. Here I implemented that by first extending the locale prefs with the option prefs, then taking the intersection of the full prefs with the locale prefs to see which ones were not changed.
| }); | ||
| } | ||
|
|
||
| fn as_unicode(&self) -> unicode::Unicode { |
There was a problem hiding this comment.
I think in some cases we have impl From Preferences for Locale
There was a problem hiding this comment.
I also thought that, but after looking at the preferences code, it seems like it was commented out:
https://github.com/unicode-org/icu4x/blob/5fb31d7ae061d304088f1e79897f7435f1846da6/components/locale_core/src/preferences/mod.rs#L519-L533
There was a problem hiding this comment.
Probably there just wasnt a need for it yet.
| fn validate(&mut self, id: &LanguageIdentifier, provider: &IntlProvider) { | ||
| self.collation_type = self.collation_type.take().filter(|co| { | ||
| let attr = DataMarkerAttributes::from_str_or_panic(co.as_str()); | ||
| co != &CollationType::Search |
There was a problem hiding this comment.
Good, this is Ecma specific functionality so it should be here in boa
There was a problem hiding this comment.
Yep, extension validation like this seems to be a good candidate for a future "icu4x-ecma402" glue crate
nekevss
left a comment
nekevss
left a comment
There was a problem hiding this comment.
This looks great! I like the new trait.
I had just a question and general thought, but nothing that's blocking
| fn as_unicode(&self) -> unicode::Unicode { | ||
| let mut exts = unicode::Unicode::new(); | ||
|
|
||
| if let Some(co) = self.collation_type |
There was a problem hiding this comment.
thought: this could probably be a macro rule
A lot of these seem to be the exact same pattern. We may be able to lower a little of the boilerplate with a macro. But I'm not entirely sure whether that will make this as clear to parse.
| provider | ||
| .locale_canonicalizer()? | ||
| .canonicalize(&mut found_locale); | ||
| let mut locale_prefs = S::Preferences::from(&found_locale); |
There was a problem hiding this comment.
question: does ordering matter here?
3 participants
A pending task after migrating to ICU4X 2.0 was to use the newly introduced locale preferences to clean up our Intl implementation. This PR takes care of that by introducing the
ServicePreferencestrait and removing theService::resolvemethod, which makes the code much more modular overall.cc @sffc, it might be interesting to see if we can upstream parts of the
ServicePreferencestrait, likeas_unicodeorintersection.