Skip to content

Rename stream_extend to extend #464

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

Merged
3 commits merged into from Nov 7, 2019
Merged

Rename stream_extend to extend #464

3 commits merged into from Nov 7, 2019

Conversation

ghost
Copy link

@ghost ghost commented Nov 6, 2019

I've renamed this method to be more consistent with the standard library and re-exported the Extend trait in our prelude.

@ghost ghost requested a review from yoshuawuyts November 6, 2019 21:02
@ghost
Copy link
Author

ghost commented Nov 6, 2019

@yoshuawuyts How do you feel about these ideas (all of them are independent of each other)?

  • We don't put our Extend into the prelude so that synchronous foo.extend(bar); still works.

  • In examples, istead of doing stream::Extend::extend(&mut foo, bar).await, first import with use async_std::stream::Extend::extend and then do extend(&mut foo, bar).await.

  • Or, perhaps even re-export a convenience function called extend into the stream module so that one can do stream::extend(&mut foo, bar).await. I kind of like that - while the std library doesn't have iter::extend because it's not needed, this reminds me of io::timeout where we can add an extra function for convenience while still keeping all APIs std already has.

We discussed this already, but am curious about another round of opinions? Simple yes/no answers will suffice :)

Copy link
Contributor

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stjepang okay with adding a new free extend method that calls the trait. I think that would solve most of the UX issues. Ok with not including it in the prelude, though still think there's a case to be made I don't feel strongly enough to push for it.

Patch seems good overall!

@ghost
Copy link
Author

ghost commented Nov 6, 2019

Here's an idea: what if both std::iter::extend() and async_std::stream::extend() existed?

That'd be nice and would make sync/async versions convenient and symmetrical at the same time :)

Let's discuss this offline one more time (briefly) and make a final decision. Thanks for clarifying motivation for this change last time we discussed it, that was helpful!

Copy link
Contributor

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ghost ghost merged commit f8e8256 into async-rs:master Nov 7, 2019
@ghost ghost deleted the rename-stream-extend branch November 7, 2019 21:46
This pull request was closed.
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.

1 participant