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

stream: add StreamExt::fuse #2085

Merged
merged 6 commits into from
Jan 10, 2020
Merged

stream: add StreamExt::fuse #2085

merged 6 commits into from
Jan 10, 2020

Conversation

carllerche
Copy link
Member

Provides an async equivalent to Iterator::fuse.

Provides an async equivalent to `Iterator::fuse`.
@hawkw
Copy link
Member

hawkw commented Jan 9, 2020

I'm a little concerned that tokio::stream::StreamExt::fuse returning a stream that does not implement FusedStream, while futures::stream::StreamExt::fuse does, may trip people up? For example, if they import tokio's version ofStreamExt, and then try to call stream.fuse() and pass the stream into futures' select! macro.

If we're going to have an identical trait name and identical method name, we should at least ensure that it's clearly documented that this is substantially different from the futures crate's StreamExt::fuse...

@LucioFranco
Copy link
Member

Agreed, but not sure what a better name might be?

@hawkw
Copy link
Member

hawkw commented Jan 9, 2020

Agreed, but not sure what a better name might be?

Well, rather than changing the name, we could just have a large and obvious note in the docs? Not my favourite solution, but better than nothing.

@carllerche
Copy link
Member Author

I don't think it is super critical to call out that it doesn't implement FuseStream... Fuse is picked as a name to match the std::Iterator::fuse impl.

@davidbarsky
Copy link
Member

I don't think it is super critical to call out that it doesn't implement FuseStream... Fuse is picked as a name to match the std::Iterator::fuse impl.

I think it should be called out. Reading this PR made made me think it did implement the fuse in futures::stream::StreamExt.

@carllerche
Copy link
Member Author

I tracked the question re: documenting the relation with FusedStream here: #1895

@carllerche carllerche merged commit cfd9b36 into master Jan 10, 2020
@carllerche carllerche deleted the stream-fuse branch January 10, 2020 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants