Skip to content

DX: remove empty calls#7138

Merged
kubawerlos merged 2 commits intoPHP-CS-Fixer:masterfrom
6b7562617765726c6f73:remove_empty
Jul 12, 2023
Merged

DX: remove empty calls#7138
kubawerlos merged 2 commits intoPHP-CS-Fixer:masterfrom
6b7562617765726c6f73:remove_empty

Conversation

@kubawerlos
Copy link
Copy Markdown
Member

Part of going to strict code.

Copy link
Copy Markdown
Member

@Wirone Wirone left a comment

Choose a reason for hiding this comment

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

IMHO it would be better if we had strict types extensions installed and errors dumped to baseline, so PRs like this would remove errors from there, while it wouldn't be possible to add new violations. Since you work on it in multiple PRs and at the same time there are other PRs ongoing, it's possible to introduce new violation for topic that was already covered. What do you think?

Comment thread src/Tokenizer/Tokens.php Outdated
{
foreach ($tokenKinds as $tokenKind) {
if (!empty($this->foundTokenKinds[$tokenKind])) {
if ($this->isTokenKindFound($tokenKind)) {
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.

this is very frequent call, we done some microoptimizations earlier for Tokens class to reduce hotpaths. maybe we do not need to call the method here

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.

If we really don't want additional method call, then it should be changed to isset($this->foundTokenKinds[$tokenKind]), just like isTokenKindFound()'s implementation was changed in #7139 (which is weird, since it's a removal of empty() call and should be here 😝). But I believe this is negligible difference and we could call the method here.

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've extracted removing empty in isTokenKindFound to separate PR as it was not that straightforward and I've expected some discussion.

What I did not expect is changes in isAnyTokenKindsFound and the other one -> I've reverted them here, so we can safely merge it and focus on Tokens in the other PR.

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.

thanks - that way this part can go fast.

i won't be heavily available in July, if we won't conclude on Tokens part fast, feel free to put it to baseline first

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.

We already did 😉 - isset as microoptimization

@kubawerlos
Copy link
Copy Markdown
Member Author

it wouldn't be possible to add new violation

@Wirone I hope to finish it withing a week (thus separating into small chunks) and if new issues will be introduced I will fix them in final PR that would add phpstan/phpstan-strict-rules into CI.

@kubawerlos kubawerlos merged commit 7195188 into PHP-CS-Fixer:master Jul 12, 2023
@kubawerlos kubawerlos deleted the remove_empty branch July 12, 2023 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants