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.

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

Add PluralRules::categories() function #789

Merged
merged 7 commits into from
Jun 14, 2021

Conversation

nordzilla
Copy link
Member

@nordzilla nordzilla commented Jun 11, 2021

Fixes #778

The categories() function returns an iterator over each
PluralCategory supported by a PluralRules object that
has been created with a given LanguageIdentifier and
PluralRuleType.

The categories are returned in alphabetical order.
This is what is expected by browsers, and the same order
that ICU4C returns.

See tc39/ecma402#578 for more info on the ordering.

@zbraniecki the above means I had to change the ordering in which the PluralCategory::all() function returns each category from numerical order to alphabetical order. This should have no impact on performance, since I doubt the numerical order is optimized for putting the most common category first (although that would be a really interesting and unlikely coincidence).

EDIT: Clearly I hadn't read the latest on the tc39 issue that I linked above. I'm happy to go back to numerical ordering based on that conversation.

@Manishearth and @sffc I know we discussed returning a struct or array of bool for FFI reasons. I'm still happy to change it to this, but returning an iterator over PluralCategory seemed, to me, to be the most clean, idiomatic and natural Rust way to write this function. This approach should also be no-alloc, since it returns an iterator of 'static references.

I am wondering if we can keep it this way, and put the iterator's contents into whatever structure is easiest for the FFI in the FFI layer. Please let me know either way. I'm happy to change it here if what I'm proposing won't work, but I think that returning an array of booleans is a pretty poor Rust-level API. That would make it entirely up to documentation to know which ordering the booleans represent (i.e. Few should always be first).

Lastly, I was unsure of whether to call this categories() or keywords().

I decided to go with categories() to match our usage in ICU4X.

The categories() function returns an iterator over each
PluralCategory supported by a PluralRules object that
has been created with a given LanguageIdentifier and
PluralRuleType.

The categories are returned in alphabetical order.
This is what is expected by browsers, and the same order
that ICU4C returns.
@coveralls
Copy link

coveralls commented Jun 11, 2021

Pull Request Test Coverage Report for Build ccaf8dbbffad13bb294add9fc627b049706079bb-PR-789

  • 16 of 16 (100.0%) changed or added relevant lines in 2 files are covered.
  • 70 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.03%) to 75.621%

Files with Coverage Reduction New Missed Lines %
components/datetime/src/provider/gregory.rs 16 74.65%
provider/cldr/src/transform/mod.rs 23 10.99%
provider/core/src/resource.rs 31 81.78%
Totals Coverage Status
Change from base Build acf388649a30f8a2c3d39fa764a36c10996c7482: -0.03%
Covered Lines: 9430
Relevant Lines: 12470

💛 - Coveralls

@nordzilla nordzilla marked this pull request as ready for review June 11, 2021 03:47
zbraniecki
zbraniecki previously approved these changes Jun 11, 2021
Copy link
Member

@zbraniecki zbraniecki left a comment

Choose a reason for hiding this comment

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

it looks good!

One nit left. I also defer to your judgement how to handle the ecma402 discussion and my opinion for the sake of this PR.
You can land it with the change, or without the change, I'm not blocking.

components/plurals/src/data.rs Outdated Show resolved Hide resolved
@@ -191,31 +191,33 @@ pub enum PluralCategory {
impl PluralCategory {
/// Returns an ordered iterator over variants of [`Plural Categories`].
///
/// Categories are returned in alphabetical order.
///
Copy link
Member

Choose a reason for hiding this comment

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

opinion: as you recognized in your PR comment, there is a discussion about the order, and in particular I am in a bad position to review this change since I expressed a strong opinion there.

I want to stress that my opinion there is non-blocking and if the group will decide to go against my recommendation I will accept it and thus do not consider it blocking for this PR. Just a change that I disagree with to match a proposal that I disagree with.

Copy link
Member

Choose a reason for hiding this comment

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

I vote to land this as-is and update it to reflect the decision in ECMA-402. I added tc39/ecma402#578 to the agenda for the July 1 meeting.

@Manishearth
Copy link
Member

I am wondering if we can keep it this way, and put the iterator's contents into whatever structure is easiest for the FFI in the FFI layer. Please let me know either way. I'm happy to change it here if what I'm proposing won't work, but I think that returning an array of booleans is a pretty poor Rust-level API. That would make it entirely up to documentation to know which ordering the booleans represent (i.e. Few should always be first).

Yeah it makes sense as is, we can do the transform at the FFI layer.

@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2021

Codecov Report

Merging #789 (113cae0) into main (acf3886) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #789      +/-   ##
==========================================
+ Coverage   75.48%   75.55%   +0.06%     
==========================================
  Files         194      192       -2     
  Lines       12297    12312      +15     
==========================================
+ Hits         9283     9302      +19     
+ Misses       3014     3010       -4     
Impacted Files Coverage Δ
components/plurals/src/data.rs 78.94% <100.00%> (+6.22%) ⬆️
components/plurals/src/lib.rs 86.95% <100.00%> (+1.95%) ⬆️
provider/cldr/src/transform/mod.rs 10.98% <0.00%> (-0.27%) ⬇️
provider/testdata/src/lib.rs
provider/testdata/src/metadata.rs
provider/testdata/src/paths.rs
provider/cldr/src/transform/dates.rs
provider/testdata/src/test_data_provider.rs
provider/cldr/src/transform/dates/symbols.rs 80.88% <0.00%> (ø)
provider/cldr/src/transform/dates/patterns.rs 79.48% <0.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acf3886...113cae0. Read the comment docs.

components/plurals/tests/fixtures/mod.rs Outdated Show resolved Hide resolved
components/plurals/src/lib.rs Outdated Show resolved Hide resolved
@nordzilla nordzilla requested a review from sffc June 14, 2021 17:19
@nordzilla nordzilla merged commit 299db8f into unicode-org:main Jun 14, 2021
@nordzilla nordzilla changed the title Add PluralRules::categories() function. Add PluralRules::categories() function Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add PluralRules Categories functionality
6 participants