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

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

Merged
merged 6 commits into from Jul 29, 2023

Conversation

JustusFluegel
Copy link
Contributor

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

Signed-off-by: Justus Fluegel <justusfluegel@gmail.com>
Signed-off-by: Justus Fluegel <justusfluegel@gmail.com>
@Testare
Copy link

Testare commented Jul 13, 2023

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

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

JustusFluegel commented Jul 14, 2023

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

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

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

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

@JustusFluegel
Copy link
Contributor Author

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

Signed-off-by: Justus Fluegel <justusfluegel@gmail.com>
Signed-off-by: Justus Fluegel <justusfluegel@gmail.com>
Signed-off-by: Justus Fluegel <justusfluegel@gmail.com>
@phimuemue
Copy link
Member

phimuemue commented Jul 19, 2023

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

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

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

Philippe-Cholet commented Jul 19, 2023

@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()
}

Signed-off-by: Justus Fluegel <justusfluegel@gmail.com>
@JustusFluegel
Copy link
Contributor Author

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

JustusFluegel commented Jul 27, 2023

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

Thank you. Hope your exam went well.

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 29, 2023

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 bors bot merged commit e0b8a12 into rust-itertools:master Jul 29, 2023
10 checks passed
@jswrenn jswrenn added this to the next milestone Nov 14, 2023
@jswrenn jswrenn mentioned this pull request Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants