Open khollbach opened 1 year ago
Not entirely sure about procedure, but is there a good reason not to merge this sort of thing?
particularly map_err
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.)
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!
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.
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.
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?
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.
This PR adds
map_err
anderr_into
methods toItertools
..map_err(f)
is just like.map_ok(f)
, but it appliesf
to eachResult::Err
value..err_into()
is simply.map_err(Into::into)
.These methods already exist on
Stream
s ("async iterators") inTryStreamExt
, and I think the ergonomics are quite pleasant. E.g., the following pattern becomes less cumbersome:I've included tests for iterator specialization, and doctests for both methods. Implementation details live in
crate::adaptors::map
, alongsideMapOk
andMapInto
.Let me know if there's any feedback or concerns; or if this feature is just not needed.
Thanks!