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.

Sign up for GitHub

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

Jump to bottom

[NodeRemover] Use return null after $this->removeNode() #3558

Merged
merged 3 commits into from
Apr 3, 2023
Merged

[NodeRemover] Use return null after $this->removeNode() #3558

merged 3 commits into from
Apr 3, 2023

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented Apr 3, 2023 •
edited
Loading

The removeNode() method calls:

        $this->nodesToRemoveCollector->addNodeToRemove($node);
        $this->rectorChangeCollector->notifyNodeFileInfo($node);

which:

  • no need to connect prev-parent-next for removed node
  • already apply RectorWithLineChange via notifyNodeFileInfo()

@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba it is ready for review.

@samsonasik
Copy link
Member Author

There is already e2e test for it https://github.com/rectorphp/rector-src/blob/main/e2e/applied-rule-removed-node/expected-output.diff

samsonasik
samsonasik commented Apr 3, 2023
View reviewed changes
rules/Php73/Rector/FuncCall/ArrayKeyFirstLastRector.php Outdated
@@ -130,7 +130,7 @@ public function refactor(Node $node): ?Node

$this->removeNode($node);

return $node;
return null;
Copy link
Member Author

Choose a reason for hiding this comment

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

this part need to be rolled back as it change next func call before remove node, so it change next node data that need to be reconnected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted with add comment note for it a5944d3

@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba I am merging it ;)

@samsonasik samsonasik merged commit 5e5681d into main Apr 3, 2023
@samsonasik samsonasik deleted the return-node branch April 3, 2023 17:15
@samsonasik samsonasik mentioned this pull request Apr 8, 2023
Merged
samsonasik added a commit that referenced this pull request May 8, 2023
@samsonasik
[NodeRemover] Use return null after $this->removeNode() (#3558)
b06e624 
* [NodeRemover] Use return null after $this->removeNode()

* return node if it try to change next node before remove

* update warning message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TomasVotruba TomasVotruba Awaiting requested review from TomasVotruba

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
1 participant
@samsonasik