Add take_while_inclusive method

[junbl]

Adds take_while_inclusive method that returns elements as long as a predicate is satisfied, but including the first element that doesn't satisfy the predicate.

First discussed addition to std in rust-lang/rust#62208.

Closes #597.

[pickfire]

As suggested in the original pull github issue in rust, wouldn't something like take_until be easier to find?

[junbl]

I don't think so. The motivation for this method is "take_while but keeping the last element". take_until doesn't imply anything about the behavior with the final element—the only thing it implies is that the semantics of the predicate are reversed, e.g.:

// take elements while they're less than 4
iter.take_while(|x| x < 4)
// take elements until one is greater than or equal to 4
iter.take_until(|x| x >= 4)

If I just read this code without reading any docs, I would probably assume these did the same thing, and certainly wouldn't guess the motivation of having both methods. The negation becomes the most noticeable difference and distracts from the actual purpose.

Plus, if I see take_while in std and I'm looking for a method that's almost that but lets me keep the last element, I'm much more likely to find take_while_inclusive than take_until. And I'd be annoyed to have to negate the predicate if I was converting one to the other.

[scottmcm]

I'm definitely used to until COND being shorthand for while !COND, like with https://www.perltutorial.org/perl-until/, not implying another iteration or similar.

[smessmer]

Are there plans to merge this?

[jswrenn]

Thanks for the ping, @smessmer. I'll give this a final round of review tomorrow morning, and merge if everything looks good.

[scottmcm]

pondering: Would it be worth returning the result of the predicate in some way in the item type, since that information is known free anyway?

I was reminded of Position::Last, though it shouldn't be that type (since this doesn't know the other variants inherently), and I suppose one could always .take_while_inclusive(…).with_position() if they wanted.

My brain then thought about things like Result<I::Item, I::Item> so the last item is an Err. But that makes me go one step further and think of a take_while_map_inclusive (hopefully with a better name!) where the predicate returns a Result (or a ControlFlow) instead of a bool, and stops after returning the Err (or the Break).

Dunno if any of that is useful or worth doing, but I figured I'd toss it out for discussion.

[smessmer]

Returning a custom type that Deref's to the actual item type but has additional information about the predicate could be a good idea.

Otherwise, the call site might be able to infer it:
In the returned iterator, all entries except the last one had the predicate return true, and the last had the predicate return false, with the following exceptions:

• If the returned iterator has the same length as the original iterator, then all elements returned true and there is no last element that returned false.
• (special case of the previous rule) If the returned iterator is empty, then the input iterator was empty and there is no last element that returned false.

Given those rules, it might also be an idea to make that information accessible from the TakeWhileInclusive iterator object in something like an "last_element_failed_predicate" boolean flag instead of having a per-item boolean flag. Wouldn't be as convenient for iterator chaining, but if the calling code needs it, they could get it if they store the iterator in an intermediate variable.

edit: nvm the proposal. The rule is correct but when instantiating theTakeWhileInclusive, we don't have that information yet.

[scottmcm]

Oh, I should have read the Iterator docs better. It looks like what I propose would be named map_while_inclusive to go with take_while_inclusive.

I'll add that to the ACP, if one gets opened.

[jswrenn]

@e-rhodes Have you tried submitting this as an addition to Iterator? As a rule, we avoid accepting proposals that could be suitable additions to Iterator, because such additions can later lead to conflicts between Itertools and Iterator (see the history of flatten and intersperse for examples of such conflicts).

[junbl]

@jswrenn In the rust-lang issue I linked, I got the impression that it wasn't deemed useful enough to be added to Iterator, and it was suggested that we add it to itertools instead (see here).

If you get a different impression and think it's likely to get accepted over there I'm happy to pr!

[jswrenn]

Hm, that comment wasn't made by a member of the libs team. I've asked on Zulip whether the libs team is interested: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Status.20of.20.2362208/near/311918204

[jswrenn]

It sounds like the next step is to create an ACP for this method. Would you like to tackle that, @e-rhodes?

[junbl]

@jswrenn sure! I'll take a look at it this weekend. Thanks for getting this started!

[junbl]

Created ACP here: rust-lang/libs-team#142

Let me know if there's anything I should add/change!

[tranzystorekk]

Could use it for todays Advent Of Code 😛

[jswrenn]

@tranzystorek-io, please let the libs team know! rust-lang/libs-team#142

[jswrenn]

There's been no movement on the libs team ACP, so I'm going to go ahead and merge this. As part of #702, we're considering prefixing the Itertools methods with it_, which would avoid the risk of conflicts with Iterator and allow us to more easily accept contributions.

Sorry for the delay!

bors r+

[bors]

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• bors build finished