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

from/into stream #125

Merged
merged 5 commits into from
Sep 17, 2019
Merged

from/into stream #125

merged 5 commits into from
Sep 17, 2019

Conversation

yoshuawuyts
Copy link
Contributor

@yoshuawuyts yoshuawuyts commented Aug 29, 2019

This adds Stream counterparts to FromIterator, IntoIterator and Iterator::collect, allowing to use the same patterns that are common in streams. Thanks!

Tasks

  • FromStream
  • IntoStream
  • Stream::collect

Screenshot

Screenshot_2019-08-29 async_std stream - Rust

@yoshuawuyts
Copy link
Contributor Author

yoshuawuyts commented Aug 29, 2019

Okay, so interesting fact here: if we actually want to provide implementations for collect here we'll have to add at least the vec, and probably also the collections submodules to hold the various IntoStream types (e.g. like vec's https://doc.rust-lang.org/std/vec/struct.IntoIter.html).

I won't add it as part of this patch, but it seems like something that we should enumerate if we land this, and probably would make for a good issue to onboard folks with!

edit: we currently can't add the IntoStream types because Stream is directly implemented on types, and it conflicts with the generic IntoStream for Stream bound. We should uhh, figure out how to fix this.

@yoshuawuyts
Copy link
Contributor Author

yoshuawuyts commented Aug 29, 2019

Noticed another thing:

This not only applies to VecDeque, but Vec as well (implements Stream directly). I'm uhhh, feeling like we're going in the right direction here.

Ideally we could also expose stream / stream_mut methods on the collections, but I think that might be more trouble than is worth it.

edit: also it's probably worth talking to boats about the trait requirements for async iteration. This will probably influence what we do here.

@yoshuawuyts yoshuawuyts marked this pull request as ready for review August 30, 2019 09:20
@yoshuawuyts
Copy link
Contributor Author

I've gone ahead and also implemented Stream::collect. Usage example:

use async_std::prelude::*;
use async_std::stream;

let s = stream::repeat(9u8).take(3);
let buf: Vec<u8> = s.collect().await;

assert_eq!(buf, vec![9; 3]);

I couldn't figure out the right trait bounds using static futures, so I just did a box bound on it. It shows up as DynFuture in the doc output. Hope that's okay!

src/stream/stream.rs Outdated Show resolved Hide resolved
@ghost ghost mentioned this pull request Sep 4, 2019
@yoshuawuyts
Copy link
Contributor Author

@stjepang could you push your changes from last Tuesday if you have time?

@ghost
Copy link

ghost commented Sep 6, 2019

@yoshuawuyts I have pushed the changes into stjepang/async-std (branch into_stream_trait) since I don't have permissions to push into your fork.

@yoshuawuyts
Copy link
Contributor Author

I had restore all the Send bounds that were removed in stjepang/async-std because the tests were failing (even if the code did compile). We've been able to keep the Unpin bounds out though. This patch should be all good now. Thanks!

@yoshuawuyts
Copy link
Contributor Author

Also note from private chat: we should introduce this feature behind the "unstable" flag.

bors bot added a commit that referenced this pull request Sep 11, 2019
145: Add Stream::poll_next r=stjepang a=stjepang

Adding `poll_next` to the `Stream` trait will simplify #125.

After a discussion with @yoshuawuyts and @withoutboats, we became confident that the `Stream` trait of the future will never solely rely on `async fn next()` and will always have to rely on `fn poll_next()`.

This PR now makes our `Stream` trait implementable by end users.

I also made a few adjustments around pinning to `all()` and `any()` combinators since they take a `&mut self`, which implies `Self: Unpin`. A rule of thumb is that if a method takes a `&mut self` and then pins `self`, we *have to* require `Self: Unpin`.

Co-authored-by: Stjepan Glavina <stjepang@gmail.com>
@yoshuawuyts yoshuawuyts force-pushed the into_stream_trait branch 2 times, most recently from bca314a to 0148df7 Compare September 17, 2019 15:59
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

update examples

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

impl collect

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

compiles!

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

layout base for collect into vec

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

fmt

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

progress

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

compiles!

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

define failing test

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

cargo fmt

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

stuck again

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

fix trait bounds!

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

cargo fmt

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

hide dyn fut impl

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

dyn ret for vec

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

cargo fmt

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

collect docs

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

remove macro from vec::from_stream

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

shorten collect trait bound

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>

Remove some Unpin and Send bounds

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
@yoshuawuyts
Copy link
Contributor Author

All feedback implemented; talked about this and we're happy to try this out as "unstable" 🎉

bors r+

bors bot added a commit that referenced this pull request Sep 17, 2019
125: from/into stream r=yoshuawuyts a=yoshuawuyts

This adds `Stream` counterparts to `FromIterator`, `IntoIterator` and `Iterator::collect`, allowing to use the same patterns that are common in streams. Thanks!

## Tasks
- [x]  `FromStream`
- [x] `IntoStream`
- [x] `Stream::collect`

## Screenshot
![Screenshot_2019-08-29 async_std stream - Rust](https://user-images.githubusercontent.com/2467194/63928985-ec2bd200-ca50-11e9-868c-9899800e5b83.png)

Co-authored-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
@bors
Copy link
Contributor

bors bot commented Sep 17, 2019

Build succeeded

  • continuous-integration/travis-ci/push

@bors bors bot merged commit 98927a7 into async-rs:master Sep 17, 2019
@yoshuawuyts yoshuawuyts deleted the into_stream_trait branch September 17, 2019 17:49
bors bot added a commit that referenced this pull request Sep 17, 2019
207: Added the ability to collect a stream of results r=yoshuawuyts a=sunjay

As requested here: https://twitter.com/yoshuawuyts/status/1174026374316773377

The standard library has a very useful implementation of `FromIterator` that takes an iterator of `Result<T, E>` values and is able to produce a value of type `Result<Vec<T>, E>`. I asked for this in `async-std` and @yoshuawuyts recommended that I contribute the impl. It turns out that the implementation in the standard library is even more general than I initially thought. It allows any collection that implements `FromIterator` to be collected from an iterator of `Result<T, E>` values. That means that you can collect into `Result<Vec<T>, E>`, `Result<HashSet<T>, E>`, etc.

I wanted to add a similarly generic impl for this crate so we can also support collecting into any collection that implements `FromStream`. 

The implementation for this is based heavily on [what exists in `std`](https://github.com/rust-lang/rust/blob/9150f844e2624eb013ec78ca08c1d416e6644026/src/libcore/result.rs#L1379-L1429). I made a new `result` module since that's where this impl is in `std`. I still wanted to maintain the conventions of this repo, so I copied the `vec` module that @yoshuawuyts created in #125. Much like in that PR, the new `result` module is private.

There is a doctest in the documentation for `collect` that both teaches that this feature exists and tests that it works in some simple cases.

## Documentation Screenshot

![image](https://user-images.githubusercontent.com/530939/65075935-de89ae00-d965-11e9-9cd6-8b19b694ed3e.png)


Co-authored-by: Sunjay Varma <varma.sunjay@gmail.com>
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.

None yet

1 participant