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

ArrayPushFixer - introduction #4790

Open
wants to merge 19 commits into
base: master
from

Conversation

@SpacePossum
Copy link
Member

SpacePossum commented Feb 7, 2020

possum@aquarium:/home/possum/work/PHP-CS-Fixer$ php php-cs-fixer describe array_push
Description of array_push rule.
Converts simple usages of ``array_push($x, $y);`` to ``$x[] = $y;``.

Fixer applying this rule is risky.
Risky when the function `array_push` is overridden.

Fixing examples:
 * Example #1.
--- Original
+++ New
@@ -1,2 +1,2 @@
 <?php
-array_push($x, $y);
+$x[] = $y;

closes #3483

@SpacePossum

This comment has been minimized.

Copy link
Member Author

SpacePossum commented Feb 7, 2020

Tagged WIP because I'm not 100% sure I covered all the operator-precedence cases. Also I wonder if there are more complex cases than the unit test cover.
Please let me know!

@SpacePossum SpacePossum force-pushed the SpacePossum:master_ArrayPushFixer branch 4 times, most recently from 49e9f6d to a05403f Feb 7, 2020
@ktomk

This comment has been minimized.

Copy link
Contributor

ktomk commented Feb 9, 2020

Please check the PR description, the code-example it gives is wrong (the result, at the very beginning, the diff looks legit).

@SpacePossum

This comment has been minimized.

Copy link
Member Author

SpacePossum commented Feb 10, 2020

thanks, corrected!

@SpacePossum

This comment has been minimized.

Copy link
Member Author

SpacePossum commented Feb 11, 2020

@ntzm can you've a look?

src/Fixer/Alias/ArrayPushFixer.php Outdated Show resolved Hide resolved
src/Fixer/Alias/ArrayPushFixer.php Outdated Show resolved Hide resolved
src/Fixer/Alias/ArrayPushFixer.php Outdated Show resolved Hide resolved
src/Fixer/Alias/ArrayPushFixer.php Outdated Show resolved Hide resolved
src/Fixer/Alias/ArrayPushFixer.php Outdated Show resolved Hide resolved
src/Fixer/Alias/ArrayPushFixer.php Outdated Show resolved Hide resolved
src/Fixer/Alias/ArrayPushFixer.php Outdated Show resolved Hide resolved
src/Fixer/Alias/ArrayPushFixer.php Outdated Show resolved Hide resolved
@SpacePossum SpacePossum force-pushed the SpacePossum:master_ArrayPushFixer branch from 643dfce to 50f65db Feb 12, 2020
@SpacePossum SpacePossum force-pushed the SpacePossum:master_ArrayPushFixer branch from c819418 to 58e4432 Feb 12, 2020
SpacePossum added 2 commits Feb 12, 2020
@gharlan

This comment has been minimized.

Copy link
Contributor

gharlan commented Feb 12, 2020

What about these cases? Are they handled? Do we want to handle them?

array_push(\A\B::$foo, 1);
array_push(static::$foo, 1);
array_push(namespace\A::$foo, 1);
array_push(foo()->bar, 1);
array_push(foo(), 1);
@SpacePossum

This comment has been minimized.

Copy link
Member Author

SpacePossum commented Feb 13, 2020

I think this is going to be a PHP7 only fixer, dealing with 5.6 is just a pain

@SpacePossum

This comment has been minimized.

Copy link
Member Author

SpacePossum commented Feb 13, 2020

What about these cases? Are they handled? Do we want to handle them?

not handled, we could, should we?

@gharlan

This comment has been minimized.

Copy link
Contributor

gharlan commented Feb 13, 2020

I think yes. We should accept anything for the first argument.

@SpacePossum

This comment has been minimized.

Copy link
Member Author

SpacePossum commented Feb 14, 2020

👍 done, however I'm not 100% sure on precedence and such

@SpacePossum SpacePossum force-pushed the SpacePossum:master_ArrayPushFixer branch from d50adcf to 3f61952 Feb 15, 2020
@SpacePossum SpacePossum removed the status/WIP label Feb 15, 2020
@SpacePossum SpacePossum added this to the 2.17.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.