[String] Add AbstractString::containsAny() #35936
Conversation
| { | ||
| $instance = static::createFromString($string); | ||
|
|
||
| $this->assertSame(null !== $instance->indexOf($needle), $instance->containsAny($needle)); |
There was a problem hiding this comment.
Does copy pasting the method content here actually test something? Shouldn't we create dedicated providers for this new methods?
There was a problem hiding this comment.
It's better this way to me, easier to maintain.
I think it's to pass directly (without alteration) the method argument to |
Clearly, for consistency. |
|
in this case does assuming we dont ever need alternatively, shouldnt we advice to compose any/all logic in userland based on |
3 extra letters adding clarity win over a new method to me, especially considering that the class is already a collection of methods. We shouldn't inflate contains*() method in comparison to the other ones.
You should have a look again at the full interface of the class: several methods already support Doing so is consistent, and useful. |
|
Note that I'm fine naming this method |
|
I think that this is the same problem we suffered for years with the |
|
there's long term, are we really, really, sure about a defacto standard String component/API... without a do we really mind adding N*3 methods (single, any, all) IF the case is ambiguous, in favor of clear behavior + type info... is it worth this debate we have? |
|
Apparently ppl don't have an issue with before/after. I suppose that's because nobody expects a "beforeAny" or "afterAny". That wouldn't mean anything useful and it intuitively is a beforeAll and afterAll. the discussion proves that "contains" can be confusing. I'm therefor confirming that I definitely also prefer Thanks for raising the point. |
|
Thank you @nicolas-grekas. |
We decided to not have a
contains()method because we didn't know how to handle the case where several needles are passed. But https://wiki.php.net/rfc/str_contains made me reconsider this idea and I thinkcontainsAny()works great. WDYT?