rust-itertools / itertools

Extra iterator adaptors, iterator methods, free functions, and macros.
https://docs.rs/itertools/
Apache License 2.0
2.64k stars 299 forks source link

[FEAT]: Conditional iterator methods #881

Closed SFM61319 closed 5 months ago

SFM61319 commented 5 months ago

As the title suggests, iterator methods that let you chain other iterator methods conditionally would be helpful. Something that lets you do:

fn print_bytes_in_order(bytes: &[u8], reverse: bool) {
    bytes
        .iter()
        // Applies `Iterator::rev` if and only if the closure (`|| reverse`) returns `true`.
        .apply_if(|| reverse, Iterator::rev)
        .for_each(|b| println!("{}", b));
}

Along with other methods for ternary conditions, etc., like:

iter
    // Applies `SomeIterator::windows` if and only if the closure (`|| overlap`) returns `true`.
    // If `false`, applies `SomeIterator::chunks`
    .apply_conditionally(|| overlap, SomeIterator::windows, SomeIterator::chunks)
    .collect();

If this feature request gets accepted, I would love to contribute to it myself.

Philippe-Cholet commented 5 months ago

Maybe I'm misunderstanding but maybe you don't need closures here? A boolean seems enough right?

It makes me think of either::Either being an iterator if both sides are (and if the item type is the same for both), see link.

let it = bytes.iter();
let new_it = if reverse { Either::Left(it.rev()) } else { Either::Right(it) };
new_it.for_each(...);

let new_it = if overlap { Either::Left(SomeIterator::windows(iter)) } else { Either::Right(SomeIterator::chunks(iter)) };
new_it.collect();

If I understand correctly, in trait Itertools it would just be:

fn apply_if<A, F>(self, condition: bool, f: F) -> Either<A, Self>
where
    Self: Sized,
    A: Iterator<Item = Self::Item>, // could be IntoIterator instead
    F: FnMut(Self) -> A,
{
    if condition { Either::Left(f(self)) } else { Either::Right(self) }
}

and something similar for apply_conditionally. Playground example.

SFM61319 commented 5 months ago

You are right, a variable should be enough here. It's simple to implement but could be useful, especially when you have a large iterator chain and breaking it up for an if[-else] block would negatively affect the readability (like in your example that does uses if-else).

I have already written a simple (and similar) implementation for the same on my local fork (I've named apply_conditionally as apply_either for now, but I am looking for better name suggestions).

Philippe-Cholet commented 5 months ago

I would prefer more intuitive names as well. But it can be discussed after an approval, apply_if[_else] maybe or something with "either" IDK.

It is simple enough and I see some potential like it.apply_if(condition, Iterator::rev). Methods with two closures is definitely more rare though but it occasionnally exists.

And it would use our only dependency either which is nice.

...But one could say that it's not limited to iterators: some_object.apply_if(condition, |obj| obj.method()) which can be useful as long as Either<L,R> has a common behavior with L and R. As you can see in either's doc, there is more than iterator as common behavior. Maybe it would be better in its own trait.

trait EitherExt {
    fn apply_if<A, F>(self, condition: bool, f: F) -> Either<A, Self>
    where
        Self: Sized,
        F: FnOnce(Self) -> A,
    {
        if condition { Either::Left(f(self)) } else { Either::Right(self) }
    }
}
impl<T> EitherExt for T {}

Or as I discovered tap only a few days ago but which seem quite popular, maybe in Tap if they added either as a dependency (eventually behind a feature flag).

...But I guess this new trait could also be integrated to either itself. It might be the ideal scenario?

Anyway I could definitely see using it myself. And I like your tuple_windows/tuples doc example.

For the names, I'm also thinking of branch_left/branch[/branch_right].

phimuemue commented 5 months ago

@Philippe-Cholet is right. Not tied to iterators. I think the either crate may be a good place.

Philippe-Cholet commented 5 months ago

@SFM61319 Please let me know where this feature ends up, it's really interesting. 🤩

SFM61319 commented 5 months ago

@Philippe-Cholet You are right, this could be widened to a larger scope instead of being limited to just iterators. I will request (and try to work on) this feature in the either crate itself.

And I'll make sure to let you know once this gets added to either (or maybe one of it's parent crates?). 😄

Anyway, thanks for your time and suggestions guys!

Philippe-Cholet commented 5 months ago

@SFM61319 Trying to simplify this.

trait EitherExt {
    fn into_either(self, left: bool) -> Either<Self, Self> // or "branch" for the name.
    where
        Self: Sized,
    {
        if left { Either::Left(self) } else { Either::Right(self) }
    }
}
impl<T> EitherExt for T {}

then we can use Either methods: obj.into_either(condition).map_left(...).map_right(...) (EDIT: or obj.into_either(condition).map_either(..., ...)). Not as short as your suggestions but maybe more canonical.

Well, I might use this in my own projects.