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

string.contains() #11439

Closed
ben-albrecht opened this issue Oct 23, 2018 · 10 comments
Closed

string.contains() #11439

ben-albrecht opened this issue Oct 23, 2018 · 10 comments

Comments

@ben-albrecht
Copy link
Member

ben-albrecht commented Oct 23, 2018

Chapel domains support a .contains() method for querying if a value is in the domain (as of #5041).

Should Chapel strings be consistent with the domain interface and support string.contains() for querying if a substring is in the string?

The string type currently supports string.find() and string.count(), however these are slightly more verbose to use in a conditional than string.contains():

if myString.find('waldo') > 0 { }
if myString.count('waldo') > 0 { }
if myString.contains('waldo') { }
@rsh3khar
Copy link
Contributor

@ben-albrecht

Any updates if this feature is to be implemented or not? If yes, I would like to work on this.

@ben-albrecht
Copy link
Member Author

@RajShekhar7 - it is not implemented, but this requires design discussion before pursuing implementation. Check out contributor info to understand that process better.

@rsh3khar
Copy link
Contributor

@ben-albrecht - I've read the Contributor Info, but help me understand, should I open a new issue for design discussions in this case if I want to implement it?

@Aniket21mathur
Copy link
Member

@RajShekhar7 I think you can give your design inputs on this thread itself.

Thanks !

@ben-albrecht
Copy link
Member Author

I've read the Contributor Info, but help me understand, should I open a new issue for design discussions in this case if I want to implement it?

This thread is fine. The big thing is that we need other core developers to agree to the design before pursuing it.

@Yudhishthira1406
Copy link
Contributor

@ben-albrecht, I would like to work on this.

This thread is fine. The big thing is that we need other core developers to agree to the design before pursuing it.

I want to discuss how to implement this as I have 1 or 2 approaches in mind, if all others agree.

@ben-albrecht
Copy link
Member Author

ben-albrecht commented Mar 10, 2021

I think the implementation for this is fairly straightforward, e.g.

proc string.contains(s: string) {
  return this.count(s) > 0;
}

The big thing is that we need other core developers to agree to the design before pursuing it. To move this forward, someone should make a post to chapel developers discourse requesting input, as instructed here: https://github.com/chapel-lang/chapel/blob/master/doc/rst/developer/bestPractices/ContributorInfo.rst#design

@e-kayrakli
Copy link
Contributor

I find the idea very appealing. And in general, find the behavior as Ben sketched out above to be the right behavior. I would like to know/discuss:

  • Examples from other languages where such a function exits, if any.
  • Would the name be confusing? "abcd".contains("bd"); I think this should return false, but there is at least one case (string.strip) where we pass a string to represent list of characters, rather than an actual string. I personally don't think that would cause confusion here. An alternative would be to name it containsSubstring or something, but I like wordiness much less than a slight possibility of confusion.
  • Edge cases: does any string contain an empty string? Does empty string contain empty string? For that, I am curious what Ben's implementation sketch above would do, and whether that behavior is confusing in the context of string.contains

@Yudhishthira1406
Copy link
Contributor

Yudhishthira1406 commented Mar 11, 2021

@e-kayrakli I did a bit of google search and found out that.

  • Python has __contains__() and Java has contains() methods.
  • An empty string is a universal substring. I cross-checked it with the python's __contains()__ function.

@ben-albrecht
Copy link
Member Author

Closing since a more recent feature request has captured the info from here and has additional info with a future checked in: #19119

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

No branches or pull requests

5 participants