Skip to content
This repository has been archived by the owner on Dec 3, 2023. It is now read-only.

[Astral] copy over findFirst() and findFirstPrevious() from rector #3876

Closed
wants to merge 4 commits into from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jan 8, 2022

it seems these methods are not tested within astral.. so I just copied the methods over from rector

refs rectorphp/rector#6921

@TomasVotruba
Copy link
Member

TomasVotruba commented Jan 8, 2022

I was lazy to write them, but test is needed for new methods. Even the old ones :)
We need a proof this works and does the job.

$this->bootKernel(AstralKernel::class);
$this->simpleNodeFinder = $this->getService(SimpleNodeFinder::class);

$phpParser = $this->getService(SmartPhpParser::class);
Copy link
Contributor Author

@staabm staabm Jan 8, 2022

Choose a reason for hiding this comment

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

I am wondering why the SmartPhpParser does not use ParentConnectingVisitor and NodeConnectingVisitor .. is this intentional?

if so, is there another parser I could use in this test which is configured for the visitors?

see https://github.com/nikic/PHP-Parser/pull/681/files

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to add them.

@staabm staabm changed the title copy over findFirst() and findFirstPrevious() from rector [Astral] copy over findFirst() and findFirstPrevious() from rector Jan 8, 2022
@TomasVotruba
Copy link
Member

What is missing here to finish this PR?

@staabm
Copy link
Contributor Author

staabm commented Jan 24, 2022

atm I don't need it, as I duplicated the logic into phpstan-dba.

I will come to this back later

@staabm staabm closed this Jan 24, 2022
@staabm staabm deleted the findprevious branch January 24, 2022 10:10
@deprecated-packages deprecated-packages locked as resolved and limited conversation to collaborators Jan 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants