-
-
Notifications
You must be signed in to change notification settings - Fork 411
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
Bump ICU4X to 1.4 and finish Intl impls with new APIs #3469
Conversation
2cacdac
to
d2bef18
Compare
d2bef18
to
8bb1b32
Compare
Test262 conformance changes
Fixed tests (2):
|
8bb1b32
to
d034bc7
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3469 +/- ##
==========================================
- Coverage 44.62% 44.59% -0.04%
==========================================
Files 487 487
Lines 50539 50601 +62
==========================================
+ Hits 22552 22563 +11
- Misses 27987 28038 +51 ☔ View full report in Codecov by Sentry. |
d034bc7
to
f52270d
Compare
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.
Looks good to me! Had a couple comments, but nothing blocking.
// 6. Let type be pluralRules.[[Type]]. | ||
// 7. Return PluralRuleSelectRange(locale, type, xp.[[PluralCategory]], yp.[[PluralCategory]]). | ||
Ok( | ||
plural_category_to_js_string(plural_rules.native.resolve_range(x.category, y.category)) |
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.
thought: Would it be worth checking if PluralCategory
could implement Display
? Then this could just be something akin to Ok(JsString::from(<PluralCategoryInput>.to_string()).into())
.
Definitely, not necessary for this PR, and maybe it's been decided against by icu4x
, but the thought popped in my head when I was looking through it.
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.
We really need a JsDisplay
trait...
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.
Looks good to me! :)
Depends on #3334, just because it removes some Intl related warnings.