Add Itertools::filter_{,map_}results

[gin-ahirsch]

I found myself wanting filter_map_results() and just implemented filter_results() in the same stretch. I hope you find it useful!

I wasn't sure if #236 applies to these new iterators; I just implemented them.

[gin-ahirsch]

I'm using {Option,Result}::transpose(), which is unavailable in 1.24 since it has been stabilized in 1.33. Should I just do it manually (using match)?

[leeola]

Is there a blocker for this? Trying to gauge acceptance of this PR, as I would like this.

[phimuemue]

Hi! As we see from #377 (comment), this is apparently a convenience feature that some people like. So thumbs up from my side.

I was thinking about the following: If I understand this correctly, filter_results is basically a convenience for a special case of filter, and I was thinking if it is possible to implement FilterResults in terms of Filter, thereby gaining all the knowledge/optimizations already put into Filter. Or would we lose anything by doing that?

[phimuemue]

I am not sure if this is correct. If self.iter iterates over e.g. a Vec, I assume that self.iter.size_hint returns vec.len(), Some(vec.len()), but due to filtering, our result should be 0, Some(vec.len()).

[gin-ahirsch]

You assessment is correct. If this implementation will be used I will fix that.

[jswrenn]

Can you fix this?

[gin-ahirsch]

I was thinking about the following: If I understand this correctly, filter_results is basically a convenience for a special case of filter, and I was thinking if it is possible to implement FilterResults in terms of Filter, thereby gaining all the knowledge/optimizations already put into Filter. Or would we lose anything by doing that?

Sounds good, should be less complex than the manual filter implementation.
Upon trying to that that I had trouble naming a valid return-type since the implementation requires that f (the filter_results function) is moved into the closure for filter.
E.g.

    fn filter_results<F, T, E>(self, f: F) -> std::iter::Filter<Self::Item, F>
        where Self: Iterator<Item = Result<T, E>> + Sized,
              F: FnMut(&T) -> bool,
    {
        self.filter(|item| item.as_ref().map(f).unwrap_or(true))
    }

results in

error[E0308]: mismatched types
   --> src/lib.rs:732:9
    |
728 |     fn filter_results<F, T, E>(self, f: F) -> std::iter::Filter<Self::Item, F>
    |                                               -------------------------------- expected `std::iter::Filter<std::result::Result<T, E>, F>` because of return type
...
732 |         self.filter(|item| item.as_ref().map(f).unwrap_or(true))
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `std::result::Result`, found type parameter
    |
    = note: expected type `std::iter::Filter<std::result::Result<T, E>, F>`
               found type `std::iter::Filter<Self, [closure@src/lib.rs:732:21: 732:64 f:_]>`

Manually implementing the Fn* traits is unstable, so using a struct instead of the generated closure also won't work.

[phimuemue]

Ah, thanks for trying - I always fall into this trap. I think if you actually wanted to use this, you would have to specify impl FnMut in return position (see https://doc.rust-lang.org/edition-guide/rust-2018/trait-system/impl-trait-for-returning-complex-types-with-ease.html#impl-trait-and-closures), but this is not supported within traits for now.

Regarding manually implementing the Fn* trait: We have a similar thing for kmerge/kmerge_by where the underlying struct basically does not expect an FnMut but instead a KMergePredicate (and each FnMut happens to be a KMergePredicate). The difference there is that KMerge is completely controlled by us, so we cannot simply apply this pattern to Filter. (Or can we?)

So, if we would like to have the feature, we should possibly go with your suggestion. Still, we could leverage Filter for e.g. fold (just as you already do for collect). Maybe this would at least unify the implementations a bit. Not sure if it is as easily doable for size_hint, though.

Alternatively:

• Should this be proposed for inclusion in std? (Such that it is possibly easier to share functionality with Filter.)
• Are we rather talking about a method that should belong to Result (possibly something like is_err_or_ok_satisfying) and can be used with filter?

[gin-ahirsch]

Regarding manually implementing the Fn* trait: We have a similar thing for kmerge/kmerge_by where the underlying struct basically does not expect an FnMut but instead a KMergePredicate (and each FnMut happens to be a KMergePredicate). The difference there is that KMerge is completely controlled by us, so we cannot simply apply this pattern to Filter. (Or can we?)

I will take a look at KMerge and KMergePredicate. Just a heads-up: I may not get around to it the next two weeks.

So, if we would like to have the feature, we should possibly go with your suggestion. Still, we could leverage Filter for e.g. fold (just as you already do for collect). Maybe this would at least unify the implementations a bit. Not sure if it is as easily doable for size_hint, though.

Indeed, where self is consumed by move this is easily doable.

Alternatively:

* Should this be proposed for inclusion in std? (Such that it is possibly easier to share functionality with `Filter`.)

I think the semantics are closely related to the other existing *_results(), so as long as they won't make it to std, I think this this doesn't have a chance.

* Are we rather talking about a method that should belong to `Result` (possibly something like `is_err_or_ok_satisfying`) and can be used with `filter`?

Well, what do you know - that already basically exists as map_or(). I'll update the code to use that instead of the map(f).unwrap_or(v) constructs.
I'm unsure if that helps in reusing the existing Iterator methods though. The predicate still needs to be contained in the closure passed to self.filter().

[jswrenn]

Just a heads-up: I may not get around to it the next two weeks.

No worries! I have a big paper deadline in the next two week, so I won't be able to closely review code, anyways.

[gin-ahirsch]

I'm using {Option,Result}::transpose(), which is unavailable in 1.24 since it has been stabilized in 1.33. Should I just do it manually (using match)?

Also, what about this?

[phimuemue]

I think we do not want to require Rust 1.33 because of this. Thus, I would use a manual `match` for now, and possibly annotate that we use `transpose` once we bump the minimum Rust version. Am Montag, 17. Februar 2020, 08:38:51 MEZ hat gin-ahirsch <notifications@github.com> Folgendes geschrieben: I'm using {Option,Result}::transpose(), which is unavailable in 1.24 since it has been stabilized in 1.33. Should I just do it manually (using match)? Also, what about this? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[jswrenn]

I think we do not want to require Rust 1.33 because of this. Thus, I would use a manual match for now, and possibly annotate that we use transpose once we bump the minimum Rust version.

Agreed!

[gin-ahirsch]

I looked at KMerge and KMergePredicate and couldn't copy their approach for this implementation.
If I haven't forgotten anything the new commits should address any other concern that was raised.

[gin-ahirsch]

I'm using Result::map(f).unwrap_or(true) again in favor of Result::map_or(true) since the latter is only supported since Rust 1.41.

[jswrenn]

I wonder if filter_{map_}ok would be more descriptive name?

[gin-ahirsch]

I wonder if filter_{map_}ok would be more descriptive name?

The current names are in line with the currently available map_results() and fold_results(). I would agree that the _ok() suffix would be a better description, however if these functions are to be named as such I would like to change the names of the existing methods as well; Aliases marked as #[deprecated] can be added for the old names for backwards compatibility.

[jswrenn]

I would agree that the _ok() suffix would be a better description, however if these functions are to be named as such I would like to change the names of the existing methods as well; Aliases marked as #[deprecated] can be added for the old names for backwards compatibility.

Let's do this.

[jswrenn]

Thanks!

bors r+

[bors]

Build succeeded:

• continuous-integration/travis-ci/push