add factor_iter() and iter() methods #91
Conversation
src/lib.rs
Outdated
| /// ``` | ||
| // TODO(MSRV): doc(alias) was stabilized in Rust 1.48 | ||
| // #[doc(alias = "transpose")] | ||
| pub fn factor_iter(self) -> impl Iterator<Item = Either<L::Item, R::Item>> { |
There was a problem hiding this comment.
I wish there was a better return type here so that e.g. the result would be a DoubleEndedIterator if both sides were, but map returns types are ugly even when they are not ineffable.
There was a problem hiding this comment.
Yeah... it can mostly be spelled out like this:
Either<
iter::Map<L::IntoIter, impl Fn(L::Item) -> Either<L::Item, R::Item>>,
iter::Map<R::IntoIter, impl Fn(R::Item) -> Either<L::Item, R::Item>>,
>... and that will be conditionally double-ended and such. Or we can add a bespoke iterator type with a cleaner type signature.
There was a problem hiding this comment.
I updated the type signature to your suggestion. It is a bit icky to expose so much of the implementation in the type signature as it would require a semver bump to change it, but I can't see a better option.
I thought about adding a bespoke iterator, but I was reluctant to polute either's otherwise minimalist name space, and it was going to be a whole pile of boilerplate to forward all the methods to the correct side to pick up any optimisations (e.g. for count or last).
There was a problem hiding this comment.
It's not a great sign when the type signature is much longer than the implementation. :)
That almost makes me want to scrap the idea, except maybe it is still valuable to have something that guides type inference to the same Item = Either<L::Item, R::Item>. If you write that map_either implementation on its own, the Left side has ambiguous R, and the Right has ambiguous L. But from a few experiments, it does seem like the compiler's type inference figures out how to unify those if you try to use it as an Iterator, where they must be the same.
src/iterator.rs
Outdated
| /// ``` | ||
| // TODO(MSRV): doc(alias) was stabilized in Rust 1.48 | ||
| // #[doc(alias = "transpose")] | ||
| pub fn factor_iter(self) -> IterEither<L::IntoIter, R::IntoIter> |
There was a problem hiding this comment.
I couldn't get the MSRV compiler to be happy with resolving that huge return type in the tests, so I added a new type for it. (I also moved the iterator stuff into a module for cleanliness.)
Let me know what you think! I'm also wondering if iter_either would be a better method name, or else should I rename the new type to something like FactoredIter? #bikeshed away...
There was a problem hiding this comment.
I'm thinking now to keep the factor method name, but I also split it into factor_into_iter, factor_iter, and factor_iter_mut.
Still not sure about IterEither, but I do like that its pronunciation is kind of close to Iterator. :)
|
@aj-bagwell any thoughts after the changes I made? |
…Eiter<L,R>> Added as a method as Iter is already implemented if both sides are iterators of the same type of item.
Based on cuviper's review add a bit more detail to the factor_iter return type so that it will implement more specific iterators (e.g. DoubleEndedIterator) if both sides do.
Co-authored-by: Josh Stone <cuviper@gmail.com>
Co-authored-by: Josh Stone <cuviper@gmail.com>
Co-authored-by: Jack Wrenn <me@jswrenn.com>
fixes #90