Skip to content

[CodeQuality] Skip not regex on SimplifyRegexPatternRector #4322

Merged
samsonasik merged 3 commits intomainfrom
skip-not-regex
Jun 22, 2023
Merged

[CodeQuality] Skip not regex on SimplifyRegexPatternRector #4322
samsonasik merged 3 commits intomainfrom
skip-not-regex

Conversation

@samsonasik
Copy link
Copy Markdown
Member

Ref rectorphp/rector#7993

@kenjis let's start with detect not regex type before check of unicode identifier.

@samsonasik
Copy link
Copy Markdown
Member Author

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

@samsonasik samsonasik merged commit e80557a into main Jun 22, 2023
@samsonasik samsonasik deleted the skip-not-regex branch June 22, 2023 19:54
@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Jun 23, 2023

@kenjis it seems RegexPatternDetector::isRegexPattern() doesn't check identifier, eg: /u or /i or /anything, so it will always skipped if regex contains identifier:

https://getrector.com/demo/4114fea0-ee00-4394-a714-b3e325a3dd8b

which regex identifier can be any of imsxeADSUXJu, it was exists in ConsistentPregDelimiterRector, just removed at

https://github.com/rectorphp/rector-src/pull/3797/files#diff-b52d5691f8efd8fdd8b301dd7b8110eb3048ca332e76619f030a6a89f5764278

The logic of ConsistentPregDelimiterRector (that removed) should be bring back to verify that

@kenjis
Copy link
Copy Markdown
Contributor

kenjis commented Jun 23, 2023

@samsonasik
Copy link
Copy Markdown
Member Author

Yes 👍

@samsonasik
Copy link
Copy Markdown
Member Author

RegexPatternDetector was designed for common delimiter used, other than that, just skipped. I think we can focus on what more important part, the identifier usage: imsxeADSUXJu

@kenjis
Copy link
Copy Markdown
Contributor

kenjis commented Jun 23, 2023

Yeah, identifiers are more important.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants