Skip to content

Add stream::Extend #211

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 22, 2019
Merged

Add stream::Extend #211

merged 2 commits into from
Sep 22, 2019

Conversation

tirr-c
Copy link
Contributor

@tirr-c tirr-c commented Sep 18, 2019

No description provided.

@ghost
Copy link

ghost commented Sep 18, 2019

Thank you for another PR!

While this is a very logical addition, I'm feeling kind of unsure of the utility of this trait. The std::iter::Extend trait is implemented for synchronous collections only and is not used for I/O in the standard library at all. So I wonder whether we should have an async equivalent or not...

Another (minor!) concern is the name of the trait. It's a bit unfortunate that the trait name is Extend, while the method name is extend_with_stream. I wish the names matched, but the ExtendWithStream trait name does not feel amazing either. :) Maybe it's okay though, idk!

@yoshuawuyts Do you have thoughts on this?

@yoshuawuyts
Copy link
Contributor

While this is a very logical addition, I'm feeling kind of unsure of the utility of this trait.

It's worth noting that Rayon has ParallelExtend trait too. I see this as a useful addition to FromStream.

In terms of uses outside of std::collections, you could imagine having a kv database that once opened can be "extended" by taking a stream of key-value tuples. There's probably some really interesting API designs that could be done, and I think accepting this PR will allow people to experiment with them.

It's a bit unfortunate that the trait name is Extend, while the method name is extend_with_stream.

Rayon calls their method par_extend. I think following their lead and calling it stream_extend might be a suitable fit. Alternatively we keep calling it extend and let people calling it deal with specifying which extend they want to use. I don't love this, but I think it's acceptable.

playground example of Extend conflicts

yoshuawuyts
yoshuawuyts reviewed Sep 18, 2019
View reviewed changes
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.

Added diffs to go from extend_with_stream to stream_extend

src/stream/extend.rs Outdated
///
/// let mut v: Vec<usize> = vec![1, 2];
/// let s = stream::repeat(3usize).take(3);
/// v.extend_with_stream(s).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// v.extend_with_stream(s).await;
/// v.stream_extend(s).await;

src/stream/extend.rs Outdated
#[cfg_attr(feature = "docs", doc(cfg(unstable)))]
pub trait Extend<A> {
/// Extends a collection with the contents of a stream.
fn extend_with_stream<'a, T: IntoStream<Item = A> + 'a>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn extend_with_stream<'a, T: IntoStream<Item = A> + 'a>(
fn stream_extend<'a, T: IntoStream<Item = A> + 'a>(

src/stream/extend.rs Outdated
}

impl Extend<()> for () {
fn extend_with_stream<'a, T: IntoStream<Item = ()> + 'a>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn extend_with_stream<'a, T: IntoStream<Item = ()> + 'a>(
fn stream_extend<'a, T: IntoStream<Item = ()> + 'a>(

src/vec/extend.rs Outdated
use crate::stream::{Extend, IntoStream, Stream};

impl<T> Extend<T> for Vec<T> {
fn extend_with_stream<'a, S: IntoStream<Item = T> + 'a>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn extend_with_stream<'a, S: IntoStream<Item = T> + 'a>(
fn stream_extend<'a, S: IntoStream<Item = T> + 'a>(

@yoshuawuyts
Copy link
Contributor

Ref #129

ghost
ghost approved these changes Sep 21, 2019
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! @yoshuawuyts, can you take one final look and merge if you're happy with the PR?

@tirr-c
Copy link
Contributor Author

tirr-c commented Sep 21, 2019

@stjepang I decided to add Extend since Iterator::unzip in std requires Extend, but I now think unzip could be implemented with FromStream bound. I'll open up a PR soon!

yoshuawuyts
yoshuawuyts approved these changes Sep 22, 2019
View reviewed changes
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.

Excellent; thanks heaps!

@yoshuawuyts yoshuawuyts merged commit a1bc097 into async-rs:master Sep 22, 2019
@yoshuawuyts yoshuawuyts mentioned this pull request Sep 22, 2019
Closed
@ghost
Copy link

ghost commented Sep 24, 2019

Note that Extend is re-exported in std's prelude: https://doc.rust-lang.org/nightly/std/prelude/v1/index.html

Should we likewise re-export our Extend in async_std's prelude? And if so, should we rename the trait to StreamExtend in order not to conflict with std's Extend and to align the name more closely with the method name stream_extend()?

@yoshuawuyts
Copy link
Contributor

Should we likewise re-export our Extend in async_std's prelude? And if so, should we rename the trait to StreamExtend in order not to conflict with std's Extend and to align the name more closely with the method name stream_extend()?

I think yes on the former, and no on the latter. The name conflicting only becomes a problem when wanting to implement it, which I don't think this is for (as long as we export it as _). It's more so people can use it, and the method name is unique already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yoshuawuyts yoshuawuyts yoshuawuyts approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tirr-c @yoshuawuyts