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 {left,right}_or_{right,left} methods to EitherOrBoth #721

Closed
wants to merge 1 commit into from

Conversation

Testare
Copy link

@Testare Testare commented Jul 31, 2023

Been using this library for a bit, wanted to create some methods to cover a use case I kept coming across: I was zipping two iterators of the same type (zip_longest), taking the value from one if it was there but wanting to default to the other if not. Thought it would be a good contribution upstream! Did my homework to make sure it wasn't a duplicate of another PR, though saw other good PR's in this space.

I considered names like either_favor_right/either_favor_left, but left_or_right/right_or_left seemed the most ergonomic IMHO. It's a simple enough change if you disagree though.

@phimuemue
Copy link
Member

If we take this, we should implement it as self.reduce(|l, _| l) resp. self.reduce(|_, r| r).

Looking at it, I am unsure if we really need this functionality. I see it's a bit more work to write it via reduce, but is it much more complex than a bespoke function? @Testare @jswrenn Do you have thoughts?

@Testare
Copy link
Author

Testare commented Aug 8, 2023

I'm fine with implementing the method that way. Reduce might cover this, though I didn't realize until a week or two before this PR that reduce was the method I was looking for. I felt like these names might better complement the methods "left", "right", and "both".

Perhaps it would be good to call out in the doc string for these methods that if you want different behavior for the both case, to look at "reduce." But if you feel this isn't a good fit for the package, I'd understand =]

@phimuemue
Copy link
Member

I updated the docs for EitherOrBoth::reduce in 052423a.

@phimuemue phimuemue closed this Aug 8, 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.

2 participants