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

number_format() Support rounding negative places #11487

Closed
wants to merge 2 commits into from

Conversation

marc-mabe
Copy link
Contributor

This PR adds support for rounding negative $decimals in number_format().

Previously negative $decimals got silently ignored and the number got rounded to zero decimal places.
With this change, when $decimals is negative, $num is rounded to $decimals significant digits before the decimal point.

This is matching the behavior for the $precision argument of round().

The previous behavior wasn't documented or tested at all.

@jorgsowa
Copy link
Contributor

Hey @marc-mab

Nice change! I am not sure what's the exact process of merging code, but shouldn't this improvement be documented in changelog somehow? It's for sure BC breaking change.

@marc-mabe
Copy link
Contributor Author

Hi @jorgsowa, yes, it should be documented somehow but I'm not sure either about the process. Was hoping to get some reviews first and then what needs to be done for documenting it.

About BC break I think this is extremely low and as the previous behavior wasn't documented or tested I would classify that into undefined behavior.

I hope this little change doesn't need an RFC.

@iluuu1994
Copy link
Member

It makes sense to match the behavior with round. You can send a mail to the internals mailing list and ask for feedback. Generally, there's an unwritten rule that without objection after a couple of weeks, a small behavioral change is acceptable. Just make sure to explicitly mention that. E.g. if there are no objections, Ilija has agreed to merge this for master. We will of course document this accordingly.

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have an objection against this change. Just test needs improving.

ext/standard/tests/math/number_format_negative_zero.phpt Outdated Show resolved Hide resolved
ext/standard/tests/math/number_format_negative_zero.phpt Outdated Show resolved Hide resolved
@marc-mabe
Copy link
Contributor Author

@bukka Thanks for your review ... I have reverted the modified test file and added a new one testing multiple cases on the $decimals param.

@marc-mabe
Copy link
Contributor Author

@bukka @iluuu1994 The two failing tests seems to be very much unrelated to this change. As I don't have the permissions to restart tests should I commit a dummy change or how is the way forward?

@iluuu1994
Copy link
Member

iluuu1994 commented Jul 5, 2023

@marc-mabe No worries, CI doesn't need to be green if it's obviously unrelated.

how is the way forward?

There hasn't been any feedback so I think we can merge this change for PHP 8.3. I'll do so in the coming days.

@iluuu1994 iluuu1994 self-assigned this Jul 5, 2023
@bukka bukka closed this in e85fb09 Jul 9, 2023
@bukka
Copy link
Member

bukka commented Jul 9, 2023

I meged it so #11584 can get rebased and reviewed in time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants