@sgiehl opened this Pull Request on February 10th 2020 Member

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)

@tsteur commented on February 10th 2020 Member

@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 commented on February 10th 2020 Member

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:

https://github.com/matomo-org/matomo/blob/b90f3fcd87682ae16384b715c0d1e43104074634/tests/PHPUnit/Integration/NumberFormatterTest.php#L84

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 :thinking:

@tsteur commented on February 10th 2020 Member

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 commented on February 12th 2020 Member

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 commented on February 12th 2020 Member

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 commented on February 13th 2020 Member

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

@tsteur commented on February 13th 2020 Member

Tests seem to pass so looks good 👍

This Pull Request was closed on February 14th 2020
Powered by GitHub Issue Mirror