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 map_err and err_into iterator adapters #714

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

khollbach
Copy link

@khollbach khollbach commented Jul 23, 2023

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!

@Philippe-Cholet Philippe-Cholet added the fallible-iterator Iterator of results/options label Jan 12, 2024
@Zynh0722
Copy link

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

particularly map_err

@scottmcm
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

scottmcm commented Feb 26, 2024

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
Copy link
Member

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 fallible-iterator Iterator of results/options , I would suggest to take a look at #844 for what we might soon do.

@phimuemue
Copy link
Member

phimuemue commented Feb 26, 2024

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fallible-iterator Iterator of results/options
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants