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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure the number of fraction digits is correct #15544

Merged
merged 2 commits into from Feb 14, 2020
Merged

Ensure the number of fraction digits is correct #15544

merged 2 commits into from Feb 14, 2020

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Feb 10, 2020

The current behavior when formatting number is a bit inconsistent when the minimum number of fraction digits is set.

Let's assume we have minimum number of fraction digits set to 3. The current fomating would do that:

number formatted number
50.001 50.001
50 50
50.1 50.100

With the suggested changes the 50 should result in 50.000. There is actually a test covering exact that case, unfortunately it doesn't fail at the moment, as the assertEquals in the PHPUnit version we are using returns true when comparing 50 and 50.000 馃し鈥嶁檪
(The assertSame would actually fail. In newer PHPUnit versions the assertEquals fails, too)

@sgiehl sgiehl added this to the 4.0.0 milestone Feb 10, 2020
@sgiehl sgiehl added the Needs Review PRs that need a code review label Feb 10, 2020
@sgiehl sgiehl marked this pull request as ready for review February 10, 2020 14:11
@tsteur
Copy link
Member

tsteur commented Feb 10, 2020

@sgiehl is there an issue for this? Looking at eg https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/38281/Dashboard_rowevolution.png it was maybe better before?

@sgiehl
Copy link
Member Author

sgiehl commented Feb 10, 2020

No there was no issue. I was preparing to update the PHPUnit version and this test suddenly started to fail as it return 50 and not 50.000 as defined in the test:

array('en', -50, 3, 3, '-50.000'),

The format function takes two parameters for minimumFractionDigits and maximumFractionDigits. Imho it is logically incorrect to return no faction digits if minimum and maximum are set to 3 but they would be zero filled 馃

@tsteur
Copy link
Member

tsteur commented Feb 10, 2020

Be just good to not regress UI. Do we have to update PHP Unit? Maybe we could wait with that unless we're after some particular feature? Of course it failing there shows there maybe is some issue which wasn't visible before but might still not be too important?

Do we otherwise need to maybe rename that parameter or add some doc to make the current behaviour more clear maybe?

@sgiehl
Copy link
Member Author

sgiehl commented Feb 12, 2020

The behavior is simply inconsistent. Imho we need to adjust the usage of the parameters. In the case you mentioned above it imho would make more sense to set minimum fraction digits to 0. So it would also return 0 instead of 0.0.0, but also 5.23 instead of 5.230.

@tsteur
Copy link
Member

tsteur commented Feb 12, 2020

In the case you mentioned above it imho would make more sense to set minimum fraction digits to 0. So it would also return 0 instead of 0.0.0, but also 5.23 instead of 5.230.

馃憤 probably need to do this in most places to keep existing behaviour probably?

@sgiehl
Copy link
Member Author

sgiehl commented Feb 13, 2020

I searched through the code, but it looks like we didn't use the minimum fraction digits count anywhere else (besides in tests)

@tsteur
Copy link
Member

tsteur commented Feb 13, 2020

Tests seem to pass so looks good 馃憤

@sgiehl sgiehl merged commit 86814cb into 4.x-dev Feb 14, 2020
@sgiehl sgiehl deleted the nbformat branch February 14, 2020 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants