Skip to content

Add try_map_results method. #283

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pthariensflame
Copy link
Contributor

This method is like the existing map_results, but allows the provided closure to potentially fail with an Err value of its own on each input. The Err values from both the original iterator and the closure will show up in the output of the adaptor.

I felt the need for this method very recently in my own code; it's implementable manually in terms of Itertools::map_results, Iterator::map, and Result::and_then, but it's both cleaner and more obviously efficient if it's its own method.

@pthariensflame
Copy link
Contributor Author

@bluss This is ready for review, whenever you have time.

@bluss
Copy link
Member

bluss commented Nov 24, 2018

@pthariensflame sorry for taking forever. Can we check if this is something that Rust's std wants to take?

@pthariensflame
Copy link
Contributor Author

I suppose. I was assuming they wouldn't, since the map_results method this ”extends” is in itertools rather than std. Is there a particular procedure for checking?

@bluss
Copy link
Member

bluss commented Nov 25, 2018

@pthariensflame Either opening a PR there or checking interest with people in the libs team. Though I see your point, this seems to fit better in here. No chance some kind of try_map would do well in std?

In std with the try_ prefix, should it really be short circuiting like try_fold and try_for_each are?

@bluss
Copy link
Member

bluss commented Nov 25, 2018

cc @Centril :) what do you think?

@Centril
Copy link
Contributor

Centril commented Nov 25, 2018

@bluss Hmm; this seems to me a shorthand for iter.map(|x| x.and_then(closure)) so idk; it might be a nice ergonomics thing... I think if it makes sense for itertools it makes sense for libcore; conversely, if it doesn't make sense for libcore it probably doesn't make sense for itertools.

@jswrenn jswrenn self-assigned this Jul 18, 2019
@pthariensflame pthariensflame force-pushed the feature/try_map_results branch from 95342cb to b3f31e7 Compare April 16, 2020 00:35
@jswrenn
Copy link
Member

jswrenn commented May 3, 2020

Sorry for the wait. Generally, I think we should stick with the core::Iterator convention that try_ is reserved for short-circuiting adaptors. I'm not opposed to ergonomic shortcuts, but this needs a more suitable name.

@pthariensflame
Copy link
Contributor Author

@jswrenn Maybe map_and_then?

@jswrenn
Copy link
Member

jswrenn commented Jun 18, 2020

@pthariensflame map_and_then sounds good to me!

@pthariensflame pthariensflame force-pushed the feature/try_map_results branch from b3f31e7 to 389acab Compare July 6, 2020 07:14
@pthariensflame
Copy link
Contributor Author

@jswrenn How's it look now?

@phimuemue
Copy link
Member

I have nothing against extending our map-convenience-function zoo, but I think we should first simplify our existing map_-variants (see #464) and express map_and_then in terms of a general map-convenience function. (Because we already have two map_-convenience adaptors that diverged in implementation, and I think we should avoid that.)

@pthariensflame I am sorry that this issue came to my mind so late. Would you also be ok with incorporating your idea once/if #464 becomes accepted?

@pthariensflame
Copy link
Contributor Author

@philmuemue I’d be more than happy to!

bors bot added a commit that referenced this pull request Jul 21, 2020
464: Unify convenience map functions r=jswrenn a=phimuemue

In response to #283: I think we should employ a uniform architecture for our `map`-specializations (as they already diverged). This PR is meant to unify the implementations so we essentially only need to parametrize the mapping function:

* Generalize `MapInto` to `MapSpecialCase` (which can be used as basis for all our map convenience functions) and define `MapInto` in terms of `MapSpecialCase`
* Specialize `collect` (was specialized on `MapOk`, so I guess we want to keep it for now)
* Define `MapOk` in terms of `MapSpecialCase`
* Simple tests for specialized `size_hint`, `fold`, `collect`
* As at this point probably all existing PRs involving `map_xyz` are conflicting anyway, move them to an own module (similar to `coalesce`)

Co-authored-by: philipp <descpl@yahoo.de>
@Philippe-Cholet Philippe-Cholet added the fallible-iterator Iterator of results/options label Jan 12, 2024
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 waiting-on-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants