-
Notifications
You must be signed in to change notification settings - Fork 98
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
Apply number formatting when using FluentNumberOptions #377
base: main
Are you sure you want to change the base?
Conversation
Changes: - Rstest crate has been added to simplify the unit tests definitions. - Add unit tests that cover the use cases that are meant to be fixed.
Hi @zbraniecki , sorry for bothering you (I saw you've got assigned as reviewer of the last PR), but no one seems to take over this issue. Could you point me to someone that could review this request? Thanks. |
These should correspond to the JS behaviour: new Intl.NumberFormat('en', { maximumFractionDigits: 3 }).format(12.34567)
// → "12.346"
new Intl.NumberFormat('en', { maximumSignificantDigits: 3 }).format(12.34567)
// → "12.3" |
Great, that makes it crystal clear. Thanks, @eemeli . However, I'd like to hear that someone will be available to review and eventually merge the code before getting into the implementation. |
I can't commit to being fast since this is a free time effort and subject to real life interruptions, but I'm here to facilitate this. |
Thanks @alerque , no rush at all! I just wanted to double-check before getting my hands dirty. |
This pull request aims to fill a gap found in the code regarding number formatting.
According to the code, 5 different format options are allowed regarding a number's length, considering rear and trailing zeroes when needed. These options are listed in the struct
FluentNumberOptions
.However, I can't figure out what is the expected difference between
FluentNumberOptions.maximum_significant_digits
andFluentNumberOptions.maximum_fraction_digits
; and betweenFluentNumberOptions.minimum_fraction_digits
andFluentNumberOptions.minimum_significant_digits
. I'll consider those redundant, and I won't implement any specific formatting unless someone tells me what's the difference.The current implementation:
https://github.com/projectfluent/fluent-rs/blob/f2033ce8340e09000ad9efccd6215b3fa5c23496/fluent-bundle/src/types/number.rs#L148C1-L165C2
Only considers the option
FluentNumberOptions.minimum_fraction_digits
, thus all the other options are simply ignored when provided. This was already reported by #368.I'd like to work on a proposal for the implementation of such missing logic. I've added a few unit tests that show what kind of formatting shall be expected when using the options. So the idea is to implement the code that make those test to pass.
Please, let me know if I'm missing something.
Notes for the reviewers: I added Rstest as a dev-dependency, if that is not acceptable, I'll rewrite the tests that I pushed.
Closes #368