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

[Php84] Add rule for RoundingMode enum #6369

Merged
merged 11 commits into from
Dec 20, 2024

Conversation

jorgsowa
Copy link
Contributor

@jorgsowa jorgsowa commented Oct 9, 2024

Adds new rule for replacing mode constants in the function round() with RoundingMode cases in PHP 8.4.

The PR is not finished, but please let me know whether it makes sense to add such a rule to the PHP8.4 ruleset.

Referenced RFC: https://wiki.php.net/rfc/correctly_name_the_rounding_mode_and_make_it_an_enum

@jorgsowa jorgsowa marked this pull request as ready for review October 11, 2024 21:45
@samsonasik
Copy link
Member

Just a quick read, if I understand correctly, the constant is not deprecated (yet) in php 8.4, it just replaced in core code, so I think it is not really needed for now.

https://3v4l.org/Z40Kp/rfc#vgit.master

when on future, the constant will be deprecated, eg: on php 8.5, we can create the rule to migrate it, if this is accepted, we need to prepare immediatelly the Downgrade RoundingMode rules.

@jorgsowa
Copy link
Contributor Author

It's unlikely that constants will be deprecated soon as it was the result of the RFC discussion. That's why I thought such a rule would help to speed up this process.

@samsonasik
Copy link
Member

I prefer to have downgrade rule instead first then, as replace constant is not required, but once it upgraded to enum, it needs to have downgrade rule to back to constant

https://github.com/rectorphp/rector-downgrade-php

@jorgsowa
Copy link
Contributor Author

Makes sense. I will create a downgrade rule then.

@jorgsowa jorgsowa force-pushed the php84/rounding-modes-enum branch from d77bfc9 to 54553da Compare December 19, 2024 23:41
@samsonasik
Copy link
Member

Thank you @jorgsowa

@samsonasik samsonasik merged commit a72a021 into rectorphp:main Dec 20, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants