Declare closures static whenever possible#1651
Declare closures static whenever possible#1651talkinnl wants to merge 11 commits intopredis:mainfrom
Conversation
… possible. - ONLY ran Rector with ONLY StaticClosureRector. NO other changes made, not even the style fixes afterwards. - StaticArrowFunctionRector did not change anything, no static arrow functions present in all the code. - Did not add Rector dependency nor config to this PR.
| "@phpstan", | ||
| "@phpunit:exclude-connected", | ||
| "@style" | ||
| ] |
There was a problem hiding this comment.
Wanna add the static_lambda to our php-cs-fixer config as well?
There was a problem hiding this comment.
Done! Running it introduces no extra changes.
TIL the php-cs-fixer can do it too, immediately going with Rector is a habit I guess. So now it's enforced thanks to the CI fixer checks. :)
- static_lambda needs to allow risky. The rule might be theoretically risky when you're passing the closures everywhere and use bind/bindTo(), but predis code has 0 bind/bindTo() calls even.
|
Feedback processed + synced my fork. As suggested, this style is now enforced by the existing php-cs-fixer setup. |
| 'increment_style' => false, | ||
| 'trailing_comma_in_multiline' => ['after_heredoc' => true, 'elements' => ['array_destructuring', 'arrays']] | ||
| 'trailing_comma_in_multiline' => ['after_heredoc' => true, 'elements' => ['array_destructuring', 'arrays']], | ||
| 'static_lambda' => true, |
There was a problem hiding this comment.
My personal preference would be to a-z order all these array keys, but added as last line to follow the existing convention. ;)
|
@tillkruss feedback has been processed. :) |
|
|
||
| if (is_nan($value)) { | ||
| return 'nan'; | ||
| } | ||
|
|
There was a problem hiding this comment.
Why is all this whitespace being removed?
|
See #1652. |
|
Based on the whitespace comment , I already wanted to make a new PR using cs fixer for you. But nice to see you already did it. 👍 (Rector has a disclaimer to run a style tool afterwards, but in this case it'd be nicer if it didn't touch unrelated lines.) |
As per the request of @tillkruss in #1642 (comment) :
Documentation reference of static closures was already nicely provided by @predakanga in that same PR:
Also: in the next PHP major release, static closures seem to get an extra optimization: https://wiki.php.net/rfc/closure-optimizations (section 2 'Stateless closure caching').
So this PR restores the static closures.
In fact, I've just ran Rector with only the StaticClosureRector rule, which not only restores the previous static closures but finds way more.
Review hint:
You might want to look at individual commits.
I have touched 0 .php files by hand, 100% of the changes are just the
StaticClosureRectorand thecomposer style:fixafterwards.For convenience, I've added a
composer testtarget which does the safe set of checks: phpstan + phpunit without connected + style dry-run.PR was carefully made with Rector, and without any AI.
Want to reproduce the Rector change which isn't pushed as part of this PR?
composer require rector/rector --devrector.php: