-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add support for one digit decimals (FORMAT_NUMBER_0, FORMAT_PERCENTAGE_0) #2525
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.