Add `map_err` and `err_into` iterator adapters

[khollbach]

This PR adds map_err and err_into methods to Itertools.

• .map_err(f) is just like .map_ok(f), but it applies f to each Result::Err value.
• .err_into() is simply .map_err(Into::into).

These methods already exist on Streams ("async iterators") in TryStreamExt, and I think the ergonomics are quite pleasant. E.g., the following pattern becomes less cumbersome:

my_stream.map(|result| result.map_err(Into::into))
my_stream.map_err(Into::into) // nice
my_stream.err_into() // even nicer

---

I've included tests for iterator specialization, and doctests for both methods. Implementation details live in crate::adaptors::map, alongside MapOk and MapInto.

Let me know if there's any feedback or concerns; or if this feature is just not needed.

Thanks!

[Zynh0722]

Not entirely sure about procedure, but is there a good reason not to merge this sort of thing?

particularly map_err

[scottmcm]

This is a place where the questions come down to clutter and discoverability and worth-it-ness, to me. Is it really clearer to have this? Would it be worth bothering writing a lint to detect the manual way -- with map + map_err -- or doing it and saying to do this instead?

Given that map_ok exists, maybe it's fine to have map_err too. But I'd also be tempted just to not have either, really.

I wish we had a better way of doing conditional bounds in return-position impl Trait. Needing the full iterator type here -- with all the forwarding and overrides and such -- makes me much less fond of these. I'd be way more willing to have them if it was possible to just have the trait return a transparent or auto-forwarded or whatever type.

(Not to mention that std::iter::Map gets a bunch of fancy implementation tricks that MapErr can't yet do, so in some cases it would actually be a pessimization to use Itertools::map_err over just calling Iterator::map with a fancier closure.)

[Zynh0722]

That sounds rather reasonable haha. I was sure there'd be good reason for it not already existing considering map_ok does exist.

It could definitely just be me not being super comfortable with iterators of results. I mostly went looking for it because I am janking anyhow errors around in some application code I wrote, and I found an open PR which served to peak my curiosity.

Thank you for your input!

[scottmcm]

Note that for things like my_stream.err_into() // even nicer, one option instead is to write a function like

fn convert_errors<T, E, F>(x: Result<T, F>) -> Result<T, E>
    where F: Into<E>
{
    x.map_err(Into::into)
}

to then make it possible to write .map(convert_errors) instead. (Demo: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3bb2dd54599c77bc08fcdf04917bdff0.)

And that way it's still a normal iter::Map type, just with a nice ZST function parameter.

---

Oh, and obligatory mention of https://doc.rust-lang.org/stable/rust-by-example/error/iter_result.html if you're consuming iterators over results.

[Philippe-Cholet]

First, I agree with scottmcm.

Skimming the code, I don't think there is a need for a new trait TryIterator since a bound like "E1: Into<E2>" seems enough to me. Probably too complicated.

As labelled as https://github.com/rust-itertools/itertools/labels/fallible-iterator, I would suggest to take a look at #844 for what we might soon do.

[phimuemue]

But I'd also be tempted just to not have either, really.

+1

Tying these special case map_err (and cousins) to Iterator makes for a less-composable API. And if there is Iterator::map_err, one could also argue for Option::map_err or (as in the proposal) Stream::map_err... and many others.

If I insist on one-liners, I'd prefer a function that curries Result::map_err so that only the Result is needed:

fn curry_map_err<U>(f: impl FnMut(E) -> E2) -> impl FnMut(Result<U, E>)->Result<U, E2> {
 move |r| r.map_err(f)
},

And use it for all mappings over results:

iterator.map(curry_map_err(f))
option.map(curry_map_err(f))
stream.map(curry_map_err(f))

// The err_into thing:
anything.map(curry_map_err(Into::into))

I get this is controversial, but imho we should ditch the special case mappers. @jswrenn @Philippe-Cholet What do you think?

[Philippe-Cholet]

I think we might want to deprecate some things once we do #844 (we might want the current discussion there) and there is no rush to deprecate them (0.13.0 already has quite some changes).

Even if your alternative is a 1-liner, I would not call it convenient.

Just pasting some playground.