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.

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

feat: Add stripos to modernize_strpos rule #8019

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

antoniovj1
Copy link

@antoniovj1 antoniovj1 commented May 16, 2024

#6419

Adds stripos to modernize_strpos rule

@coveralls
Copy link

coveralls commented May 16, 2024

Coverage Status

coverage: 95.653% (-0.06%) from 95.715%
when pulling 7c503cf on antoniovj1:master
into 731fdd2 on PHP-CS-Fixer:master.

yield 'leading namespace' => [
'<?php if (\str_starts_with($haystack5, $needle) ) {}',
'<?php if (\strpos($haystack5, $needle) === 0) {}',
];

yield 'case insensitive leading namespace' => [
'<?php if (\str_starts_with(\strtolower($haystack5), \strtolower($needle)) ) {}',
Copy link
Author

Choose a reason for hiding this comment

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

I've added namespaces to strtolower because str_starts_with has it. Does that make sense?

@antoniovj1
Copy link
Author

@mvorisek You don't like the implementation or the rule itself (or both 🤣)?

If it is the implementation, feel free to comment, it is the first time that I work on this type of thing and there are probably errors😅

If it is about the rule, you may be able to contribute something in the issue #6419

@mvorisek
Copy link
Contributor

mvorisek commented May 21, 2024

I agree stripos is not much readable but the rule will generate slower code... It should be probably under an option and not enabled by default.

@antoniovj1
Copy link
Author

antoniovj1 commented May 21, 2024

Makes sense @mvorisek ! I will add the possibility to configure it

@Wirone Do you have some proposals for configuration option name?

@antoniovj1 antoniovj1 marked this pull request as draft May 21, 2024 20:34
@antoniovj1
Copy link
Author

@mvorisek @Wirone Finally I have added a configuration called modernize_stripos, disabled by default 👍

@antoniovj1 antoniovj1 marked this pull request as ready for review May 25, 2024 12:32
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.

None yet

3 participants