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

Change String#includes? for multiple substrings #6360

Closed
wants to merge 2 commits into from
Closed

Change String#includes? for multiple substrings #6360

wants to merge 2 commits into from

Conversation

delef
Copy link
Contributor

@delef delef commented Jul 10, 2018

Example:
"Bar".includes?('a', 'r') # => true or "Crystal".includes?("Cry", "al") # => true
but
"Crystal".includes?("Cry", "ol") # => false

@@ -1020,10 +1025,10 @@ describe "String" do
end

it "reverses taking grapheme clusters into account" do
reversed = "noël".reverse
Copy link
Member

Choose a reason for hiding this comment

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

Why is there a change in these lines?

@straight-shoota
Copy link
Member

I don't see a value in this overload. string.includes?(a, b) is by all means equal to {a, b}.all? { |part| string.includes?(part) }, which in contrast expresses clearly what's happening. Reading includes?(a, b) makes me wonder if it's using a logical conjunction or disjunction. My first guess would have been the latter.

Unless it was implemented with improved performance, I don't think a multiple string overload makes any sense.

@delef
Copy link
Contributor Author

delef commented Jul 10, 2018

@straight-shoota ok 🙂

@delef delef closed this Jul 10, 2018
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

2 participants