Add support for one digit decimals (FORMAT_NUMBER_0, FORMAT_PERCENTAGE_0)#2525
Add support for one digit decimals (FORMAT_NUMBER_0, FORMAT_PERCENTAGE_0)#2525oleibman merged 15 commits intoPHPOffice:masterfrom
Conversation
|
Might you be able to add unit tests which demonstrate that your new constants work as expected? |
To be honest: I don't fully understand, how NumberFormats are tested currently and what the existings tests actually test. But my guess is that with tests/PhpSpreadsheetTests/Style/NumberFormatTest.php that already happens automagically (but I don't fully understand, what PhpOffice\PhpSpreadsheet\Style\NumberFormat\Formatter::toFormattedString() does), |
|
Agreed that the current NumberFormat tests don't provide a particularly good template for you to decide how to add tests for your change. Here's what I'd like to see you add to tests/data/Style/NumberFormat.php: use PhpOffice\PhpSpreadsheet\Style\NumberFormat
// result, value, format (note change in order in this comment)
...
[
'yourExpectedResult',
'yourInputValue',
NumberFormat::FORMAT_NUMBER_0,
],Obviously, substitute suitable values for yourExpectedResult and yourInputValue. I would expect to see at least one test for a number with no decimal portion, with one decimal digit, and with more than one decimal digit; possibly make one of the tests format zero or a negative number. I would expect a similar set of tests for FORMAT_PERCENTAGE_0. |
|
Tests added. I've skipped an obvious one, as this one fails: However, also this one fails with the same error, so I would say it's not on my code: The failed test suggests that the formatting per se is OK, as it is a masking, not a rounding, but why does Pass the test then and is rounded? To me it looks like .5 is rounded down instead of up if masking is used. |
|
l would like to research your test failures. They seem similar to problems which appeared to be fixed by PR #2399. I agree that your change is an innocent bystander here, but let's see if we can figure out what's going wrong and fix it. |
|
OK. Tests are in https://github.com/nohn/PhpSpreadsheet/tree/strange_rounding_format_value_with_mask. Run with those tests failing: https://github.com/nohn/PhpSpreadsheet/actions/runs/1795815772 |
|
The earlier PR fixed Style/NumberFormat/NumberFormatter.php. It did not address PercentageFormatter.php, which is why you are seeing these new problems (and why adding the tests turns out to have been a better idea than expected). Can you try making the same sort of change to PercentageFormatter to see if it helps? At the end, instead of: return sprintf($mask, $value);Substitute the following: /** @var float */
$valueFloat = $value;
return sprintf($mask, round($valueFloat, $decimalPartSize)); |
|
Thanks! That seems to fix it. To keep this PR focused on the new number formats, I've create a separate one for the bugfix: #2555 |
… into one_digit_numbers_pr
…_digit_numbers_pr
… into one_digit_numbers_pr
|
Made it to the finish line at last. Thank you for your contribution. |
|
You're welcome. |
This is:
Checklist:
Why this change is needed?
Because numbers are often rounded to one digit.