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

PluralOperands::n should become a getter #283

Closed
filmil opened this issue Sep 30, 2020 · 1 comment · Fixed by #284
Closed

PluralOperands::n should become a getter #283

filmil opened this issue Sep 30, 2020 · 1 comment · Fixed by #284
Assignees
@filmil
Labels
C-pluralrules Component: Plural rules T-enhancement Type: Nice-to-have but not required
Milestone
ICU4X 0.1

Comments

@filmil
Copy link
Contributor

filmil commented Sep 30, 2020

This is based on the Slack conversation.

tl;dr: we established that keeping a public field n: f64 that is derived from what essentially are fixed point data represented with integers is a source of potential precision loss, which isn't justified, given that the field isn't used.

While we could do more to make the API even nicer, an easy change we can do right away is to change n to be a getter.

This is a backwards incompatible change, which we should be OK with as all potential uses are in the icu4x repository.
@filmil filmil self-assigned this Sep 30, 2020
@filmil
Copy link
Contributor Author

filmil commented Sep 30, 2020

@sffc @zbraniecki FYI

Feel free to correct if I got something wrong here.

@sffc sffc added C-pluralrules Component: Plural rules T-enhancement Type: Nice-to-have but not required labels Oct 1, 2020
@sffc sffc added this to the ICU4X 0.1 milestone Oct 1, 2020
filmil added a commit to filmil/icu4x that referenced this issue Oct 1, 2020
@filmil
Makes PluralOperands::n into a getter
4be33ec 
The field `n` is not used for plural selection, and may cause
precision loss in cases of conversion.  So we stop keeping in
around and compute it on demand instead.

Fixes unicode-org#283
@filmil filmil mentioned this issue Oct 1, 2020
Merged
filmil added a commit that referenced this issue Oct 1, 2020
@filmil
Makes PluralOperands::n into a getter (#284)
e6579f8 
* Makes `PluralOperands::n` into a getter

The field `n` is not used for plural selection, and may cause
precision loss in cases of conversion.  So we stop keeping in
around and compute it on demand instead.

Fixes #283

* fixup: simplify eval logic, clean up lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@filmil filmil

Labels
C-pluralrules Component: Plural rules T-enhancement Type: Nice-to-have but not required
Projects
None yet
Milestone
ICU4X 0.1
Development

Successfully merging a pull request may close this issue.

Makes PluralOperands::n into a getter filmil/icu4x
2 participants
@filmil @sffc