at_most_one should have a different return type

[pickx]

at_most_one() validates that an iterator has no more than one element, then returns it if the iterator had exactly one element, or some result that indicates that the iterator was empty. If it had more than one element, the original iterator is "restored" and returned back to the user. This is a useful concept that I occasionally need.

That said, I believe the API of the itertools implementation is not as ergonomic as it could be. The existing implementation (#523) is based on Itertools::exactly_one() and returns a Result<Option<Self::Item>, ExactlyOneError<Self>>, where ExactlyOneError yields the same elements as the original iterator and also implements Error.

This return type has 2 flaws that keep me from using this function:

1. The "good" result (Ok()) is not necessarily that the iterator had one element or zero elements. I often want to check that the iterator had exactly one element (and use it), but also behave differently in the zero elements case and in the many elements case (so in that case, zero elements is also "bad"). Similarly, having more than one element is not necessarily the only error case, it's just a different case. In other words, the zero/one/more than one distinction is better represented with three states and not two.
2. The return type uses Result<Option>, which is somewhat annoying to use.

I purpose changing the return type to something like

enum AtMostOneResult<I: Iterator> {
    Zero,
    One(I::Item),
    MoreThanOne(MoreThanOne<I>),
}

where MoreThanOne yields the same elements as the original iterator.

I have implemented this feature (+ tests, docs) and will open a PR. #702 talks about making breaking changes, so this may be an appropriate time for this.

[phimuemue]

Hi there, thanks for the feedback.

I admit that sometimes having a custom result type is better, but I found that sticking to Rust's Result is very often better. It e.g. allows simply using the contained Ok-type on its own. For at_most_one, I consider a Result<Option<Item>, ...> canonical.

That said, I'd appreciate if you could show what's exactly problematic with at_most_one. I usually use it like this:

    match it.at_most_one() {
        Ok(None) => println!("no elements"),
        Ok(Some(t)) => println!("one element: {}", t),
        Err(mut err) => println!("more than one element: {}", err.join(", ")),
    }

And I think this the above is not your proposal:

    match it.at_most_one() {
        Zero => println!("no elements"),
        One(t) => println!("one element: {}", t),
        MoreThanOne(mut err) => println!("more than one element: {}", err.join(", ")),
    }

I hope I don't miss the forest for the trees, but I think the zero/one/more is well-represented by our current API, in particular if the function's name is at_most_one. Thus, I am reluctant to change #708 .

Please let me know if my view has any flaws.

[pickx]

After taking some time to use the current at_most_one() implementation, in conjunction with exactly_one(), I'm inclined to agree with you. When using both together, I feel like my usecases are covered quite well.

As you said, using Result gives you all the nice things that Result already has. Ok(Some(t)) is actually quite succinct, sinceAtMostOneResult, like any other enum name, is fairly long and I probably won't be doing use AtMostOneResult::*.

Semantically, since the function's name is at_most_one, it also makes more sense that Ok(either_zero_or_one) would be the return type and not Zero/One/MoreThanOne.

Thank you for taking the time to consider my suggestion.