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 Iterator::map_while #65864

Closed
wants to merge 1 commit into from
Closed

Add Iterator::map_while #65864

wants to merge 1 commit into from

Conversation

timvermeulen
Copy link
Contributor

This PR adds the map_while iterator method, that transforms elements until a transformation fails:

let iter = (0..).map_while(|x| std::char::from_digit(x, 16));
assert!(iter.eq("0123456789abcdef".chars()));

There are different ways of explaining what map_while is:

  • it's like take_while, but instead of just taking elements, it maps them (hence the name)
  • it's like filter_map except that it stops on the first None
  • it's similar to while_some from itertools: iter.map(f).while_some()
  • it's a stateless version of scan: iter.scan((), |(), x| f(x))

An alternative name could be take_while_map because it is to take_while what filter_map is to filter and what find_map is to find, but to me map_while is more descriptive and less confusing.

I'm submitting this PR because I semi-regularly see someone ask how to do this, and they usually end up with something like

iter.map(f).take_while(Option::is_some).map(Option::unwrap)

If this is considered worth adding I'll create a tracking issue. If not, I'm happy to add it to itertools instead.

@rust-highfive
Copy link
Contributor

r? @rkruppe

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 27, 2019
@jonas-schievink jonas-schievink added A-iterators Area: Iterators T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 27, 2019
@hanna-kruppe
Copy link
Contributor

I have wanted an operation like this in the past, but I am unsure whether it's best placed in the standard library when itertools exists. But I'm just one random gal so if others want it in std, I won't object.

However, I feel like the map(f).while_some() alternative is at least as good -- especially if your iterator of Options isn't produced by a map. We do have both forms for some other iterator methods (flat_map vs flatten, filter_map vs filter), but we should still consider whether we want map_while or while_some or perhaps both. Adding both requires stronger justification, so it's perhaps not the best place to start.

Likewise, if we don't end up adding either one to std, then it might not make so much sense to add map_while to itertools, as it's kinda redundant. But this is up to the itertools maintainers.

Personally, I have a slight preference for while_some just because the naming works out better. while_* methods suggest a boolean predicate, and while_some strongly hints at that predicate (Option::is_some), and map_while and proposed variants aren't so explicit about it. In particular, when I had read the PR description but not the code yet, I was unsure whether it would work with Options or Results.

@timvermeulen
Copy link
Contributor Author

@rkruppe Thanks for your insight! I wouldn't mind adding while_some instead, having at least one of the two in std would be very welcome.

However, I feel like the map(f).while_some() alternative is at least as good -- especially if your iterator of Options isn't produced by a map.

while_some is absolutely better when you already have an iterator of Options, but in my experience those are almost always the result of an earlier map since it's the primary way to change the type that is iterated over.

while_* methods suggest a boolean predicate, and while_some strongly hints at that predicate (Option::is_some), and map_while and proposed variants aren't so explicit about it.

Good point, and this is why I'm a bit hesitant about the name. Couldn't the same argument be made with filter_map though? filter suggests A -> bool and map suggests A -> B, which nicely comes together as A -> Option<B>. The map_while name could be justified in the same way.

@hanna-kruppe
Copy link
Contributor

Good point, and this is why I'm a bit hesitant about the name. Couldn't the same argument be made with filter_map though? filter suggests A -> bool and map suggests A -> B, which nicely comes together as A -> Option<B>. The map_while name could be justified in the same way.

It may just be familarity, but at the end of the day filter_map does not confuse me in the same way 🤷‍♀

I would like to hear some other opinions, e.g. from @rust-lang/libs members or anyone else who happens to watch this PR. Otherwise, I still lean slightly towards requesting while_some (bikeshed: take_while_some?) instead.

@withoutboats
Copy link
Contributor

Sort of dubious about adding this to std, but I think the API proposed in the PR is more consistent with the existing filter_map API than adding while_some would be.

@JohnCSimon
Copy link
Member

ping from triage:
This pr has sat for a few days, is there a way to movie this forward?
@timvermeulen @rkruppe @withoutboats
thank you.

@KodrAus
Copy link
Contributor

KodrAus commented Nov 3, 2019

I would personally also lean towards map_while over while_some. Shall we start with itertools and circle back to std after some usage?

@hanna-kruppe
Copy link
Contributor

Ok, since others seem at best on the fence about putting this in std too, I'd go with @KodrAus's suggestion of getting this into itertools first. Still, thank you for the PR, @timvermeulen!

@timvermeulen
Copy link
Contributor Author

Shall we start with itertools and circle back to std after some usage?

@KodrAus That seems to be (somewhat) against itertools's contribution policy:

For new features, please first consider filing a PR to rust-lang/rust, adding your new feature to the Iterator trait of the standard library, if you believe it is reasonable. If it isn't accepted there, proposing it for inclusion in itertools is a good idea. The reason for doing is this is so that we avoid future breakage as with .flatten().

Keeping their policy in mind, I would prefer to not add this to itertools unless std definitely doesn't want it. Is there anything that I can do to help figure this out, such as write an RFC or an internals thread?

@hanna-kruppe
Copy link
Contributor

Hm, that sounds like a decent enough reason for an RFC. There is some conflict here, though, since widespread usage in the wild is one good argument for std inclusion. On the other hand, I don't expect this will ever be a very widespread method, so perhaps that will never become a relevant factor?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-iterators Area: Iterators S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants