rust-itertools / itertools

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

Add conversion into (Option<A>,Option<B>) to EitherOrBoth #713

Closed JustusFluegel closed 1 year ago

JustusFluegel commented 1 year ago

Hey!

I recently found a conversion into (Option<A>,Option<B>) quite useful. Just wondered if there was any interest adding this directly to upstream here. If not, just close this pr :)

Thanks

Testare commented 1 year ago

I was literally about to cut an issue for this! I've been using this library for a little while, but every time I use zip_longest I seem to bump up against the problem where I can't just operate on left if it exists and right if it exists. If not an explicit conversion, at least an "either()" function that would convert it to this type just like it has left(), right(), and both().

scottmcm commented 1 year ago

Maybe have it be an into_option method instead of a From impl? That's what MinMaxResult uses for this same operation: https://docs.rs/itertools/latest/itertools/enum.MinMaxResult.html#method.into_option.

JustusFluegel commented 1 year ago

I am open to all options, I just think a Into trait may be more generic, I for example like to sometimes write something like

fn some_fn<A,B>(param_a: impl Into<(Option<A>,Option<B>)>) -> ...

and a into impl would then allow to accept EitherOrBoth as well as other structs that implement the corresponding Into trait. Maybe that's just me tho. Would be open to add a method as well or switch it to a method only, If anyone has good points for either option feel free to add them :) I can't make the final decision tho, I am not a maintainer here.

scottmcm commented 1 year ago

Hmm, is there anything else that's Into<(Option<_>, Option<_>)>?

I understand impl Into<Cow<str>> parameter types and such (though I'm usually not a fan), but tuple-of-options seems like something that wouldn't really have very many types that it can accept -- the only thing that comes to mind is there's a new one from array of options, but that's only relevant in homogeneous situations.

Philippe-Cholet commented 1 year ago

What about fn left_and_right(self) -> (Option<A>, Option<B>) ? It has a similar signature to left and right (and both is already taken). Or left_right.

JustusFluegel commented 1 year ago

I mean having both and then rust delegating the into impl to the method (maybe with #[inline(always)]) wouldn't hurt either, right?

JustusFluegel commented 1 year ago

I'll just add the method for now, I think left_and_right is quite nice

phimuemue commented 1 year ago

Hi there, thanks a lot for this.

Please remove the impl From, and just keep the fn left_and_right, which is imho nicely aligned with the existing fn left and fn right.

And please use @Philippe-Cholet 's implementation: self.map_any(Some, Some).or_default().

Side note: I guess we'd ideally want to be able to write something like eitherorboth.map_or_default(Some), but this is - to the best of my knowledge - impossible for now. If anybody has an idea how we can make this work in stable rust, please leave a note.

scottmcm commented 1 year ago

Re the side note: You're thinking that in terms of needing a generic closure, so that Some could be both A -> Option<A> and B -> Option<B>?

phimuemue commented 1 year ago

Re the side note: You're thinking that in terms of needing a generic closure, so that Some could be both A -> Option<A> and B -> Option<B>?

Exactly. Admittedly, Rust allows me to define this:

impl<L, R> EitherOrBoth<L, R> {
    fn map_or_default<F, L2, R2>(self, mut f: F) -> (L2, R2)
        where
            F: FnMut(L)->L2 + FnMut(R)->R2,
            L2: Default,
            R2: Default,
    {
        match self {
            EitherOrBoth::Left(l) => (f(l), Default::default()),
            EitherOrBoth::Right(r) => (Default::default(), f(r)),
            EitherOrBoth::Both(l, r) => (f(l), f(r)),
        }
    }
}

But the above function cannot be called (at least I did not find any straightforward way). (In stable Rust.)

Philippe-Cholet commented 1 year ago

@phimuemue I suppose we could go with eitherorboth.map_or_default(Some, Some) instead. After all, we could theorically want to do eitherorboth.map_or_default(Some, Stuff) (EDIT: something like #[derive(Default)] struct Stuff<T>(T);).

Then, looking at or_default and without the .map_or_default(Some, Some) above, I just realized that it could be this below, right?!

pub fn left_and_right(self) -> (Option<A>, Option<B>) {
    self.map_any(Some, Some).or_default()
}
JustusFluegel commented 1 year ago

I think that should satisfy both requests, sorry that I haven't responded in a while, had to study for one of my exams ( Uni ) and wasn't online during that time.

JustusFluegel commented 1 year ago

I don't know if that would be a good idea but we could theoretically change the pub fn left(self) -> Option<A> and pub fn right(self) -> Option<B> impl to use self.left_and_right().0 and self.left_and_right().1 as well, if that is desired.

Edit:

Or .map_left(Some) and .map_right(Some) would work as well and would be cleaner imo.

Edit no 2

nevermind that wouldn't work

phimuemue commented 1 year ago

Thank you. Hope your exam went well.

bors r+

bors[bot] commented 1 year ago

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.