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

Jump to bottom

Implement Intl.PluralRules #3298

Merged
merged 5 commits into from Sep 25, 2023
Merged

Implement Intl.PluralRules #3298

merged 5 commits into from Sep 25, 2023

Conversation

jedel1043
Copy link
Member

@jedel1043 jedel1043 commented Sep 24, 2023 •
edited

This Pull Request implements the Intl.PluralRules service. It also implements some number formatting tools that will be useful when we eventually implement Intl.NumberFormat.

Intl.PluralRules.prototype.selectRange needs support from ICU4X first, but I'm in charge of it and I'll try to finish it soon™. You can track its progress on unicode-org/icu4x#3012.

@jedel1043 jedel1043 added enhancement New feature or request builtins PRs and Issues related to builtins/intrinsics labels Sep 24, 2023
@jedel1043 jedel1043 added this to the v0.18.0 milestone Sep 24, 2023
@jedel1043 jedel1043 requested a review from a team September 24, 2023 11:19
@github-actions
Copy link

Test262 conformance changes

Test result main count PR count difference
Total 95,574 95,574 0
Passed 75,084 75,156 +72
Ignored 19,494 19,494 0
Failed 996 924 -72
Panics 0 0 0
Conformance 78.56% 78.64% +0.08%
Fixed tests (72): 
test/intl402/PluralRules/name.js [strict mode] (previously Failed)
test/intl402/PluralRules/name.js (previously Failed)
test/intl402/PluralRules/builtin.js [strict mode] (previously Failed)
test/intl402/PluralRules/builtin.js (previously Failed)
test/intl402/PluralRules/prop-desc.js [strict mode] (previously Failed)
test/intl402/PluralRules/prop-desc.js (previously Failed)
test/intl402/PluralRules/proto-from-ctor-realm.js [strict mode] (previously Failed)
test/intl402/PluralRules/proto-from-ctor-realm.js (previously Failed)
test/intl402/PluralRules/constructor-options-throwing-getters.js [strict mode] (previously Failed)
test/intl402/PluralRules/constructor-options-throwing-getters.js (previously Failed)
test/intl402/PluralRules/length.js [strict mode] (previously Failed)
test/intl402/PluralRules/length.js (previously Failed)
test/intl402/PluralRules/can-be-subclassed.js [strict mode] (previously Failed)
test/intl402/PluralRules/can-be-subclassed.js (previously Failed)
test/intl402/PluralRules/default-options-object-prototype.js [strict mode] (previously Failed)
test/intl402/PluralRules/default-options-object-prototype.js (previously Failed)
test/intl402/PluralRules/internals.js [strict mode] (previously Failed)
test/intl402/PluralRules/internals.js (previously Failed)
test/intl402/PluralRules/prototype/builtins.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/builtins.js (previously Failed)
test/intl402/PluralRules/prototype/bind.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/bind.js (previously Failed)
test/intl402/PluralRules/prototype/properties.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/properties.js (previously Failed)
test/intl402/PluralRules/prototype/select/name.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/select/name.js (previously Failed)
test/intl402/PluralRules/prototype/select/prop-desc.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/select/prop-desc.js (previously Failed)
test/intl402/PluralRules/prototype/select/length.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/select/length.js (previously Failed)
test/intl402/PluralRules/prototype/select/non-finite.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/select/non-finite.js (previously Failed)
test/intl402/PluralRules/prototype/select/tainting.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/select/tainting.js (previously Failed)
test/intl402/PluralRules/prototype/toStringTag/toString-removed-tag.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/toStringTag/toString-removed-tag.js (previously Failed)
test/intl402/PluralRules/prototype/toStringTag/toString-changed-tag.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/toStringTag/toString-changed-tag.js (previously Failed)
test/intl402/PluralRules/prototype/toStringTag/toStringTag.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/toStringTag/toStringTag.js (previously Failed)
test/intl402/PluralRules/prototype/toStringTag/toString.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/toStringTag/toString.js (previously Failed)
test/intl402/PluralRules/prototype/constructor/prop-desc.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/constructor/prop-desc.js (previously Failed)
test/intl402/PluralRules/prototype/constructor/main.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/constructor/main.js (previously Failed)
test/intl402/PluralRules/prototype/resolvedOptions/name.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/resolvedOptions/name.js (previously Failed)
test/intl402/PluralRules/prototype/resolvedOptions/order.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/resolvedOptions/order.js (previously Failed)
test/intl402/PluralRules/prototype/resolvedOptions/pluralCategories.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/resolvedOptions/pluralCategories.js (previously Failed)
test/intl402/PluralRules/prototype/resolvedOptions/prop-desc.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/resolvedOptions/prop-desc.js (previously Failed)
test/intl402/PluralRules/prototype/resolvedOptions/builtins.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/resolvedOptions/builtins.js (previously Failed)
test/intl402/PluralRules/prototype/resolvedOptions/length.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/resolvedOptions/length.js (previously Failed)
test/intl402/PluralRules/prototype/resolvedOptions/properties.js [strict mode] (previously Failed)
test/intl402/PluralRules/prototype/resolvedOptions/properties.js (previously Failed)
test/intl402/PluralRules/supportedLocalesOf/name.js [strict mode] (previously Failed)
test/intl402/PluralRules/supportedLocalesOf/name.js (previously Failed)
test/intl402/PluralRules/supportedLocalesOf/arguments.js [strict mode] (previously Failed)
test/intl402/PluralRules/supportedLocalesOf/arguments.js (previously Failed)
test/intl402/PluralRules/supportedLocalesOf/prop-desc.js [strict mode] (previously Failed)
test/intl402/PluralRules/supportedLocalesOf/prop-desc.js (previously Failed)
test/intl402/PluralRules/supportedLocalesOf/length.js [strict mode] (previously Failed)
test/intl402/PluralRules/supportedLocalesOf/length.js (previously Failed)
test/intl402/PluralRules/supportedLocalesOf/main.js [strict mode] (previously Failed)
test/intl402/PluralRules/supportedLocalesOf/main.js (previously Failed)
test/intl402/PluralRules/supportedLocalesOf/supportedLocalesOf.js [strict mode] (previously Failed)
test/intl402/PluralRules/supportedLocalesOf/supportedLocalesOf.js (previously Failed)

@codecov
Copy link

codecov bot commented Sep 24, 2023 •
edited

Codecov Report

Patch coverage: 4.46% and project coverage change: -0.41% ⚠️

Comparison is base (7e18da6) 50.21% compared to head (bb29381) 49.80%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3298      +/-   ##
==========================================
- Coverage   50.21%   49.80%   -0.41%     
==========================================
  Files         436      441       +5     
  Lines       42691    43078     +387     
==========================================
+ Hits        21438    21456      +18     
- Misses      21253    21622     +369
Files Changed Coverage Δ
boa_engine/src/builtins/intl/collator/mod.rs 5.55% <ø> (ø)
boa_engine/src/builtins/intl/collator/options.rs 0.00% <0.00%> (ø)
boa_engine/src/builtins/intl/date_time_format.rs 3.06% <ø> (ø)
boa_engine/src/builtins/intl/list_format/mod.rs 5.66% <ø> (ø)
...oa_engine/src/builtins/intl/list_format/options.rs 0.00% <ø> (ø)
boa_engine/src/builtins/intl/locale/mod.rs 19.52% <ø> (ø)
boa_engine/src/builtins/intl/locale/options.rs 0.00% <ø> (ø)
boa_engine/src/builtins/intl/locale/utils.rs 52.77% <ø> (ø)
..._engine/src/builtins/intl/number_format/options.rs 0.00% <0.00%> (ø)
...oa_engine/src/builtins/intl/number_format/utils.rs 0.00% <0.00%> (ø)
... and 11 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nekevss
nekevss reviewed Sep 24, 2023
View reviewed changes
Copy link
Member

@nekevss nekevss left a comment

Choose a reason for hiding this comment

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

Most of this is looking good to me, but there's some observations I had re: ecma402 and temporal/intl shared concepts.

boa_engine/src/builtins/intl/number_format/options.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/intl/number_format/utils.rs Show resolved Hide resolved
@jedel1043 jedel1043 requested review from nekevss and a team September 24, 2023 23:00
nekevss
nekevss approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@nekevss nekevss left a comment

Choose a reason for hiding this comment

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

Overall looks good to me 😄 nice work! I had one nit, but I'd be fine with merging either way.

boa_engine/src/builtins/options.rs Outdated Show resolved Hide resolved
@nekevss nekevss requested a review from a team September 25, 2023 01:15
HalidOdat
HalidOdat approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

Nice work! Looks great to me! :)

@jedel1043 jedel1043 added this pull request to the merge queue Sep 25, 2023
Merged via the queue into main with commit 25c120b Sep 25, 2023
10 checks passed
@jedel1043 jedel1043 deleted the plural-rules branch September 25, 2023 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HalidOdat HalidOdat HalidOdat approved these changes

@nekevss nekevss nekevss approved these changes

Assignees
No one assigned
Labels
builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request
Projects
None yet
Milestone
v0.18.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jedel1043 @HalidOdat @nekevss