Skip to content

[PhpParser] Remove BetterNodeFinder::findFirstPreviousOfTypes()#4327

Merged
TomasVotruba merged 1 commit intomainfrom
php-parser-prev-of-types
Jun 23, 2023
Merged

[PhpParser] Remove BetterNodeFinder::findFirstPreviousOfTypes()#4327
TomasVotruba merged 1 commit intomainfrom
php-parser-prev-of-types

Conversation

@samsonasik
Copy link
Copy Markdown
Member

No description provided.

@samsonasik samsonasik requested a review from TomasVotruba as a code owner June 23, 2023 14:08
@samsonasik
Copy link
Copy Markdown
Member Author

All checks have passed 🎉 @TomasVotruba I think it is ready 👍

@TomasVotruba
Copy link
Copy Markdown
Member

Perfect, thank you 👏

@TomasVotruba TomasVotruba merged commit db88bc3 into main Jun 23, 2023
@TomasVotruba TomasVotruba deleted the php-parser-prev-of-types branch June 23, 2023 16:07
}

/**
* @api
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As this method was api before, should we at least mark it as deprecated in a few releases before dropping it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

findFirstPrevious() still available -- for now --, it basically just shortcut with only pass type instead of callable filter.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO for good DX we would change api only when really required.

Even if this method is no longer used in rector-src it might have been used by people in custom rules. Therefore I think we should keep it but mark it as deprecated for at least a few releases, before dropping it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I will let @TomasVotruba decide on that if it going to be reverted.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this method was rarely used, so its ok. Also refactoring to parent nodes should start as soon as possible, to get other packages who use it into new shape.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants