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

x/exp/slices: add ContainsFunc #53983

Closed
GRbit opened this issue Jul 21, 2022 · 10 comments
Closed

x/exp/slices: add ContainsFunc #53983

GRbit opened this issue Jul 21, 2022 · 10 comments

Comments

@GRbit
Copy link

GRbit commented Jul 21, 2022

During the design of the slices package ContainsFunc function was dropped at the time of discussion (#47203 (comment)). Briefly, the discussion was about the signature of the function and how often it is used.

I suggest finally making ContainsFunc, since we have Contains, so it will be consistent to have ContainsFunc as well. Also, it's just convenient to have such a function. Talking about the signature, for me, it's reasonable to make it the same as for IndexFunc: ContainsFunc[E comparable](s []E, f func(E) bool) bool

Note:
I've created a PR for this change because I'm new here and didn't know that such change needs a proposal. In case you are curious – here it is: golang/exp#39

@GRbit GRbit added the Proposal label Jul 21, 2022
@ianlancetaylor ianlancetaylor changed the title slices: add ContainsFunc proposal: x/exp/slices: add ContainsFunc Jul 21, 2022
@gopherbot gopherbot added this to the Proposal milestone Jul 21, 2022
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jul 21, 2022
@josharian
Copy link
Contributor

josharian commented Aug 11, 2022

This operation is very common. It also often goes by the name Any. Yes, ContainsFunc matches the pattern in the package, but Any is delightfully concise.

It'd also be really nice to have the dual, All. Yes, like any dual, it is easy to implement All in terms of Any, but given the frequency of use, it's worth having the clarity of a separate function.

@rsc
Copy link
Contributor

rsc commented Oct 26, 2022

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
Copy link
Contributor

rsc commented Nov 2, 2022

It sounds like the main justification would be that we have Index and Contains (which wraps Index), and we have IndexFunc but not ContainsFunc. So adding ContainsFunc would complete the set.

@bradfitz reports that in the Tailscale source tree there are four implementations of ContainsFunc, only one generic. So that argues for adding ContainsFunc.

I am sympathetic to @josharian's comment about Any, but we already call it ContainsFunc elsewhere in the standard library, so we should probably keep that name here.

Sorry, I confused myself with #54386. We don't have it already. But probably keeping the naming pattern going does make sense.

@rsc
Copy link
Contributor

rsc commented Nov 2, 2022

<tangent>
As a tangent regarding "any", if we did want to add a function called Any in Go, I think it would be nice if we could make it return (T, ok) rather than just a bool, so that you can actually extract the thing that matches the predicate. Perhaps it would be

func Any[T any](x []T, f func(T)bool) (T, bool)

or perhaps it would be

func Any[T, U any](x []T, f func(T)(U, bool)) (U, bool)

The latter is most analogous to what Common Lisp calls 'some'.

Writing out those type signatures, I notice the collision between 'Any' and 'any', which have very different meanings. That makes me think maybe the function should have a different name. Maybe Some.

If anyone (someone?) wants to push further on this, feel free to file a separate proposal issue.
</tangent>

@josharian
Copy link
Contributor

I'd like to push a little bit to consider using Any and All, since they are such nice, succinct names. And if we use ContainsFunc, it's not at all clear what to call All.

I'd call the (T, ok) flavor of this First, since that also makes it clear which element gets returned in case multiple match. (Last might also be desirable.)

@GRbit
Copy link
Author

GRbit commented Nov 7, 2022

Personally, I would like to see these signatures:

func Any[T any](x []T, f func(T)bool) (bool)
func All[T any](x []T, f func(T)bool) (bool)

It's pretty convenient to use Any in if conditions and switch cases, which would be less convenient if it would return something more than a single bool value.

First and Last functions look good to me, can implement them as well.

@rsc
Copy link
Contributor

rsc commented Nov 16, 2022

I appreciate the elegance of Any and All, but we have the analogy already set up: Index is to Contains as IndexFunc is to ___. The answer is clearly ContainsFunc. We should use the name people already familiar with the other APIs will expect if we're going to do this.

(In strings and bytes, we also have IndexAny and ContainsAny, which is something else entirely.)

@rsc
Copy link
Contributor

rsc commented Nov 16, 2022

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

@rsc
Copy link
Contributor

rsc commented Nov 30, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: x/exp/slices: add ContainsFunc x/exp/slices: add ContainsFunc Nov 30, 2022
@rsc rsc modified the milestones: Proposal, Backlog Nov 30, 2022
bradfitz added a commit to tailscale/tailscale that referenced this issue Dec 6, 2022
x/exp/slices now has ContainsFunc (golang/go#53983) so we can delete
our versions.

Change-Id: I5157a403bfc1b30e243bf31c8b611da25e995078
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit to tailscale/tailscale that referenced this issue Dec 6, 2022
x/exp/slices now has ContainsFunc (golang/go#53983) so we can delete
our versions.

Change-Id: I5157a403bfc1b30e243bf31c8b611da25e995078
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
@ianlancetaylor
Copy link
Contributor

This work was completed by https://go.dev/cl/417374.

coadler pushed a commit to coder/tailscale that referenced this issue Feb 2, 2023
x/exp/slices now has ContainsFunc (golang/go#53983) so we can delete
our versions.

Change-Id: I5157a403bfc1b30e243bf31c8b611da25e995078
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
@dmitshur dmitshur modified the milestones: Backlog, Unreleased Jun 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

6 participants