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

proposal: strings: provide ContainsOnly function #46385

Closed
ghost opened this issue May 26, 2021 · 9 comments
Closed

proposal: strings: provide ContainsOnly function #46385

ghost opened this issue May 26, 2021 · 9 comments

Comments

@ghost
Copy link

ghost commented May 26, 2021

strings package has ContainsAny. I have seen many package implement thier ContainsOnly. Although it takes just a few line of code, it would be nice to have it in strings package.

@gopherbot gopherbot added this to the Proposal milestone May 26, 2021
@mvdan
Copy link
Member

mvdan commented May 26, 2021

This proposal lacks detail. What is a ContainsOnly? Can you share links to those "many packages" that implement their own? How does it compare to ContainsAny?

@mvdan mvdan added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label May 26, 2021
@ghost
Copy link
Author

ghost commented May 26, 2021

Can you share links to those "many packages" that implement their own?

It looks like i lied. I only saw two packages implement it and both of which are for semantic versioning and they use it to check if s only contains alphanum and -. Although it may have other uses.

I am sorry.

What is a ContainsOnly?
How does it compare to ContainsAny?

It may look like this.

// ContainsAny reports whether s only contains Unicode codepoints which are in chars

func ContainsOnly(s, chars string) bool {
	if len(s) == 0 || len(chars) == 0 {
		return false
	}
outer:
	for _, r := range s {
		for _, c := range chars {
			if r == c {
				continue outer
			}
		}
		return false
	}
	return true
}

// or

func ContainsOnly(s, chars string) bool {
	if len(s) == 0 || len(chars) == 0 {
		return false
	}
	for _, r := range s {
		if strings.ContainsRune(chars, r) == false {
			return false
		}
	}
	return true
}

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) May 26, 2021
@ianlancetaylor ianlancetaylor removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label May 26, 2021
@ianlancetaylor ianlancetaylor changed the title Proposal: strings: provide ContainsOnly function proposal: strings: provide ContainsOnly function May 26, 2021
@ianlancetaylor
Copy link
Contributor

I think this could be written as strings.Trim(s, chars) == "".

Does this really come up often enough to deserve its own function? Can you point to some existing code that would be simpler if this function were available? Thanks.

@rsc
Copy link
Contributor

rsc commented Jun 9, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) Jun 9, 2021
@earthboundkid
Copy link
Contributor

This comes up, e.g., to test that all characters in an email address are valid, but I think that doing an O(n m) loop is wrong when you can use a lookup map instead.

@icholy
Copy link

icholy commented Jun 9, 2021

@carlmjohnson for small char strings, the O(n m) version might be faster.

@rsc
Copy link
Contributor

rsc commented Jun 16, 2021

The downside of using ContainsOnly for testing a character set is that the character set is very large. Passing "abcdefghijklmnopqrstuvwxyzABCDEFGHJKLMNOPQRSTUVWXYZ0123456789" and making the function figure out how to do that seems much worse (and less efficient) than just writing 'a' <= c && c <= 'z' || 'A' <= c && c <= 'Z' || '0' <= c && c <= '9'. It's also much harder to read. Did you notice the missing I in the string?

It's even worse if Unicode is involved.

It doesn't seem like we have a compelling use case here.

@rsc
Copy link
Contributor

rsc commented Jul 14, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Jul 14, 2021
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Jul 21, 2021
@rsc
Copy link
Contributor

rsc commented Jul 21, 2021

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Jul 21, 2021
@golang golang locked and limited conversation to collaborators Jul 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

6 participants