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’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

Merged
merged 15 commits into from
Feb 5, 2022

Conversation

nohn
Copy link
Contributor

@nohn nohn commented Jan 24, 2022

This is:

- [ ] a bugfix
- [x] a new feature

Checklist:

Why this change is needed?

Because numbers are often rounded to one digit.

@oleibman
Copy link
Collaborator

oleibman commented Feb 3, 2022

Might you be able to add unit tests which demonstrate that your new constants work as expected?

@nohn
Copy link
Contributor Author

nohn commented Feb 3, 2022

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
tests/data/Style/NumberFormat.php and
src/PhpSpreadsheet/Style/NumberFormat/Formatter.php

that already happens automagically (but I don't fully understand, what PhpOffice\PhpSpreadsheet\Style\NumberFormat\Formatter::toFormattedString() does),

@oleibman
Copy link
Collaborator

oleibman commented Feb 3, 2022

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.

@nohn
Copy link
Contributor Author

nohn commented Feb 4, 2022

Tests added. I've skipped an obvious one, as this one

    [
        '1.2%',
        '0.0115',
        NumberFormat::FORMAT_PERCENTAGE_0,
    ],

fails:

1) PhpOffice\PhpSpreadsheetTests\Style\NumberFormatTest::testFormatValueWithMask with data set #121 ('1.2%', '0.0115', '0.0%')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1.2%'
+'1.1%'

/home/sebastian/Projects/oss/PHPOffice/PhpSpreadsheet/tests/PhpSpreadsheetTests/Style/NumberFormatTest.php:50

2) PhpOffice\PhpSpreadsheetTests\Writer\Html\HtmlNumberFormatTest::testFormatValueWithMask with data set #121 ('1.2%', '0.0115', '0.0%')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1.2%'
+'1.1%'

/home/sebastian/Projects/oss/PHPOffice/PhpSpreadsheet/tests/PhpSpreadsheetTests/Writer/Html/HtmlNumberFormatTest.php:180

However, also this one fails with the same error, so I would say it's not on my code:

    [
        '1.12%',
        '0.01115',
        NumberFormat::FORMAT_PERCENTAGE_00,
    ],

The failed test suggests that the formatting per se is OK, as it is a masking, not a rounding, but why does

    [
        '1.2%',
        '0.0116',
        NumberFormat::FORMAT_PERCENTAGE_0,
    ],

Pass the test then and is rounded? To me it looks like .5 is rounded down instead of up if masking is used.

@oleibman
Copy link
Collaborator

oleibman commented Feb 4, 2022

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.

@nohn
Copy link
Contributor Author

nohn commented Feb 4, 2022

@oleibman
Copy link
Collaborator

oleibman commented Feb 4, 2022

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));

@nohn
Copy link
Contributor Author

nohn commented Feb 5, 2022

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

@nohn nohn mentioned this pull request Feb 5, 2022
5 tasks
@oleibman oleibman merged commit 454c01b into PHPOffice:master Feb 5, 2022
@oleibman
Copy link
Collaborator

oleibman commented Feb 5, 2022

Made it to the finish line at last. Thank you for your contribution.

@nohn
Copy link
Contributor Author

nohn commented Feb 5, 2022

You're welcome.

@nohn nohn deleted the one_digit_numbers_pr branch February 5, 2022 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants