SE-0197 — Add in-place remove(where:)

I’m sure s.remove(" ") would be a hit.

Ah yes, maybe. Though maybe s.remove { $0.isWhitespace } could be similarly popular.

(or even s.remove(where: \.isWhitespace) someday... but I'll keep my dreams of gratuitous syntactic sugar for another time)

3 Likes

Thanks Nate. Putting it in context with the other removes does seem to suggest removeAll(where:) could be a better name.

I also think including a removeAll(where predicate: (Index, Element)->Bool) variant would be useful.

There are some StackOverflow links here and here showing some poor implementations of removing multiple indices for anyone interested.

1 Like

What is your evaluation of the proposal?

+1

Is the problem being addressed significant enough to warrant a change to Swift?

Yes, I've been writing such remove(where:) extensions pretty much in every project I worked on. Having this in stdlib would be great!

Does this proposal fit well with the feel and direction of Swift?

Definitely.

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

N/A

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

A quick read of the proposal.

What is your evaluation of the proposal?

+1 for the functionality.

I agree with Nate’s argument for naming it removeAll.

I’m also bugged by the inconsistency that’s bugging Erica; filterInPlace would also make sense (with the condition swapped, obviously). However, John’s reply makes sense to me.

Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?

It’s an obvious and annoying oversight in the stdlib that I’ve repeatedly had to implement myself, probably ineffeciently.

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

Ruby provides a method quartet (mutating / non-moutating; true keeps / true removes) for this:

select / select!
reject / reject!

(The exclamation point in Ruby is part of the method name, and by convention indicates mutation or other “dangerous” methods.)

It’s surprisingly helpful for code readability to have both select and reject in the toolbox. It allows concise method refs where a closure would otherwise be required, and more importantly the name difference is often more conspicuous than the not operator. They help code read better. (I like to alias contains(where:) as any / all for similar reasons.)

However, given Swift’s precedent of using filter, I don’t see a similar name quartet that reads as nicely. In a green field, I might propose this Swifty parallel to Ruby:

selectingAll(where:) / selectAll(where:)
removingAll(where:) / removeAll(where:)

…or maybe:

keepingAll(where:) / keepAll(where:)
removingAll(where:) / removeAll(where:)

…but this is a far departure, and I don't think it warrants a change to the library. The duo (not quartet) named as proposed seems good enough to me, if not ideal.

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

A glance.

+1 for removeAll(where:) for the reasons noted by Nate (@nnnnnnnn) and others.

I'm with @Paul_Cantrell. I'd prefer to add four new keywords that follow the Swift API guidelines than try to fight an industry term of art to wrestle the right behavior into the language by pairing filter with removeAll. (Thanks @nnnnnnnn for the improved naming btw.)

This isn't quite a "if you were designing it from scratch" situation but it's close. And in such case, the utility of positive and negative logic versions is evident. I'd want to filter a sequence but maybe I'd prefer to selectAll , selectingAll, removeAll, removingAll an array, set, or dictionary.

The proposal mentions concrete implementations for Set and Dictionary, but those do not seem to be part of the implementation. Are those coming as well? This seems especially useful for Dictionary:

dict.remove[All] { isOdd($0.value) }

Good point. I'll add them to the PR.

What is your evaluation of the proposal?

+1

The following point is a bit orthogonal to the performance aspect, or improving the readability of filter meaning "keep", but I wanted to point out that if all you're looking to do is change a "keep" filter into a "remove" one then the readability of nums = nums.filter { !isOdd($0) } could be slightly improved by extending the ! operator to act on Bool returning functions, e.g.

prefix func !<D>(_ fn: @escaping (D) -> Bool) -> (D) -> Bool { return { !fn($0) } }

Then the example reduces to:

var nums = [1,2,3,4,5]
// remove odd elements
nums = nums.filter(!isOdd)

In-fact this may be useful for avoiding writing a closure when using this new in-place remove(where:) when simply the negation of a function is desired.

To remove even numbers, you could write nums.remove(where: !isOdd) for example.

That said, I think there is enough value in the performance aspect, and the convenience of being an in-place removal if dealing with an already mutable collection to warrant adding this new method.

Is the problem being addressed significant enough to warrant a change to Swift?

Yes.

Does this proposal fit well with the feel and direction of Swift?

Yes.

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

A couple of quick reads though.

2 Likes

Any more comments before review concludes? Currently it seems like consensus is in favor in adding the operation, but spelled removeAll(where:).

4 Likes

+1 on the proposal and I also support the spelling removeAll(where:), as the method is basically a constrained version of removeAll().

Once SE-0174 lands, could this method also be added to Sequence where Filtered == Self? That should cover Dictionary and Set and be useful for some user-defined types as well.

1 Like

The review of this proposal is now over. The core team has decided to accept the proposal with the revision that the method be named removeAll(where:).

In this thread, we also discussed whether to add a removeAll(_: Element) method. While I think that's a reasonable-sounding idea, I do think it can considered independently of SE-0197, since it wasn't originally part of the pitch. If you're interested in adding that method, I encourage you to start a new pitch thread for it.

1 Like

I'm a bit late for this one, Just want to point out that we now have:

drop(while:)
and the new
removeAll(where:)

both are somehow related and I'd love it we'll eventually, at some point, end up with drop(where:) too, or removeAll(while:)

Sequence.drop(while:) walks the elements in order and stops at the first disqualifying one, while Collection.removeAll(where:) conceptually checks all elements and ignores their iteration order. So, giving each method an overload for the other one's mode wouldn't make sense.

1 Like

I think @krzyzanowskim just wanted to say that we have two different verbs for methods that get rid of elements -- and probably was fooled by one name, which isn't dropping(while:) as you might expect because of our rules.
I think both names make sense in isolation, but it seems arbitrary that one operation exists only in a form that creates a new object, and the other only has a mutating variant.

thanks, that was exactly my point. thanks!

I understand that English grammar is important, but sometimes I feel like Swift is one step too far with the naming ;)

I hate to bump an old thread, but seems like removeAll(where:) is missing in both Dictionary and Set as of 5.3. Cannot find the discussion of why their implementations were dropped.

Were they ever on Dictionary and Set? The proposal talks about RangeReplaceableCollection which those two are not.

It does look like they were in the proposal but omitted from the implementation:

Since Dictionary and Set would benefit from this functionality as well, but are not range-replaceable, they should be given concrete implementations for consistency.

We recently adopted rules that such missing parts of implementations would expire after a year. I don't recall if there was a stumbling block in implementing these or if it was just missed. I'll check and if it's a simple omission, should be an easy re-propose.

1 Like