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

Either crate style methods for EitherOrBoth #327

Merged
merged 1 commit into from
Oct 1, 2019

Conversation

Avi-D-coder
Copy link
Contributor

@Avi-D-coder Avi-D-coder commented Dec 23, 2018

I have recently been using zip_longest. Without Functors or lens it's a bit of a pain.
@bluss's either crate has methods to make dealing with either variants nicer, so I added some of them. I attempted to make the naming and implementation as similar as possible, but there are a few differences.

A fallible method of converting EitherOrBoth into an either would be very useful providing exclusive (not including Both variant) methods. Converting should probably wait for rust-lang/rust#33417.

@bluss
Copy link
Member

bluss commented Dec 30, 2018

I think the defintion of is_left is well-intentioned but in practice it becomes a confusing conflict with has_left. I would for that reason leave those methods out.

@Avi-D-coder
Copy link
Contributor Author

Would you consider keeping them with names like only_left, only_is_left or using the prefix just_?
That way those of us that need this functionality don't have use an additional crate containing just:

trait IsDirection {
    fn is_left(&self) -> bool;
    fn is_both(&self) -> bool;
    fn is_right(&self) -> bool;
}

impl<a, b> IsDirection for EitherOrBoth<a, b> {
    fn is_left(&self) -> bool {
        match *self {
            Left(_) => true,
            _ => false,
        }
    }

    fn is_right(&self) -> bool {
        match *self {
            Right(_) => true,
            _ => false,
        }
    }

    fn is_both(&self) -> bool {
        self.as_ref().both().is_some()
    }
}

This brings up an issue, the merits of an either_or_both crate.
Obviously their must be a cut off, If people end up needing all of either's methods and trait implementations then a separate crate would be a good idea. As it stands now I don't think people use EitherOrBoth enough to justify a crate or the even including all the methods in either.

The only method not included in this pull is that would advocate be included is combine . I prefer the name reduce for this method. It serves as a sort of analog to the either() method.

impl<T> EitherOrBoth<T, T> {
    pub fn combine<F>(self, f: F) -> T
    where
        F: FnOnce(T, T) -> T,
    {
        match self {
            Left(a) => a,
            Right(b) => b,
            Both(a, b) => f(a, b),
        }
    }
}

What are your thoughts on combine/reduce?
Would you like any other methods to be added to this pull request?

@Avi-D-coder
Copy link
Contributor Author

@bluss If you get a chance can you take a look at this again?

@jswrenn jswrenn self-assigned this Jul 18, 2019
@Avi-D-coder
Copy link
Contributor Author

Bump

@jswrenn
Copy link
Member

jswrenn commented Sep 30, 2019

Sorry for taking so long to get to this!

Would you consider keeping them with names like only_left, only_is_left or using the prefix just_?

After some soul searching, I've decided that the benefits of sticking with the is_variant naming convention outweigh the potential for conflation with has_{left,right}.

Could you revert f5bfc64, squash, and rebase on master?

@Avi-D-coder
Copy link
Contributor Author

Should be ready to go

@jswrenn
Copy link
Member

jswrenn commented Oct 1, 2019

bors r+

bors bot added a commit that referenced this pull request Oct 1, 2019
327: Either crate style methods for EitherOrBoth r=jswrenn a=Avi-D-coder

I have recently been using `zip_longest`. Without Functors or lens it's a bit of a pain. 
@bluss's [either crate](https://crates.io/crates/either) has methods to make dealing with either variants nicer, so I added some of them. I attempted to make the naming and implementation as similar as possible, but there are a few differences.

A fallible method of converting `EitherOrBoth` into an `either` would be very useful providing exclusive (not including `Both` variant) methods. Converting should probably wait for rust-lang/rust#33417.

Co-authored-by: Avi ד <avi.the.coder@gmail.com>
@bors
Copy link
Contributor

bors bot commented Oct 1, 2019

Build succeeded

@bors bors bot merged commit d077195 into rust-itertools:master Oct 1, 2019
This was referenced Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants