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

Why does StreamExt::filter and StreamExt::filter_map require functions that returns futures #1987

Closed
jannickj opened this issue Nov 23, 2019 · 4 comments
Labels
A-stream Area: futures::stream C-question

Comments

@jannickj
Copy link

jannickj commented Nov 23, 2019

Currently looking at the api spec the functions for StreamExt::filter takes a function that must return a future, however in 99% of cases you just want to filter on the data, so why needlessly bloat the code with future::ready, instead of having a dedicated function if this is really necessary (which i doubt).

I can understand it being useful to have a function which takes futures, but filter and filter_map are very unexpected to require futures.

It just feels like someone was a bit trigger happy on the merge button.

@jannickj jannickj changed the title Why does SteeamExt::filter and SteeamExt::filter_map require functions that returns futures Why does StreamExt::filter and StreamExt::filter_map require functions that returns futures Nov 23, 2019
@tgiddings
Copy link

It's less of a requirement and more of an allowance. A non-future callback can be transformed to a future callback easily. The reverse is generally impossible. Changing these functions to use non-future callbacks would greatly reduce their capabilities.

You could make a case for separate filter* and filter*_async methods. I'm not sure the split is worthwhile though.

@jannickj
Copy link
Author

jannickj commented Jul 19, 2020

The thing is there are tons of ways to solve that problem, example: .and_then([async code]).filter_map(|x| x), the problem I have with this api design is that it's inconsistent and makes the code harder to parse because filter_map's behavior is different when applied to a stream.

@ghost
Copy link

ghost commented Jul 20, 2020

If I may make a shameless plug, this is one of my gripes with the futures crate that have pushed me to fork it: https://docs.rs/futures-lite This fork is just smaller with minor differences and compiles faster, but is still compatible with futures.

Some functionality is still missing (like filter_map()) but I'll add the remaining missing stream combinators over the next couple days. I also don't like async closures in stream combinators, which is why e.g. my fold() doesn't take an async closure, unlike the fold() method in futures.

@taiki-e
Copy link
Member

taiki-e commented Jun 18, 2023

Closing in favor of #2755.

@taiki-e taiki-e closed this as completed Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stream Area: futures::stream C-question
Projects
None yet
Development

No branches or pull requests

3 participants