add stream::peekable #366

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

Jump to bottom

Draft

starsheriff wants to merge 8 commits into async-rs:main

from starsheriff:stream_peekable

Draft

add stream::peekable #366

starsheriff wants to merge 8 commits into async-rs:main from starsheriff:stream_peekable

Conversation

starsheriff

Copy link

Contributor

@starsheriff starsheriff commented Oct 17, 2019 •

edited

Loading

Currently everything is breaking. I am still trying to figure out the type. I guess that peek should return a Future

pub fn peak(&mut self) -> &PeekFuture<Option<Self::Item>>

ref #129

starsheriff added 2 commits

October 17, 2019 23:36

@starsheriff


 add stream::peekable

8f2ed48

peek() implementation is still missing

@starsheriff


 Merge branch 'master' into stream_peekable

4f4a017

@starsheriff starsheriff force-pushed the stream_peekable branch from ecf962e to a2dc753 Compare

October 17, 2019 22:13

starsheriff added 3 commits

October 18, 2019 00:15

@starsheriff


 change type of peeked

a2dc753

@starsheriff


 match next in poll_next

d644eee

@starsheriff


 fix types and it compiles

0aef382

yjhmelody

yjhmelody reviewed

Oct 23, 2019

View reviewed changes

src/stream/stream/peekable.rs Outdated

}

None => {

let next = futures_core::ready!(self.as_mut().stream().poll_next(cx));

match next {

Copy link

Contributor

@yjhmelody yjhmelody Oct 23, 2019

Choose a reason for hiding this comment

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

match is redundant.

Copy link

Contributor

@yjhmelody yjhmelody Oct 23, 2019

Choose a reason for hiding this comment

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

Poll::Ready(next)

yjhmelody

yjhmelody reviewed

Oct 23, 2019

View reviewed changes

src/stream/stream/peekable.rs

Some(_) => {

let v = Poll::Ready(self.as_mut().peeked().take().unwrap());

*self.as_mut().peeked() = None;

Copy link

Contributor

@yjhmelody yjhmelody Oct 23, 2019

Choose a reason for hiding this comment

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

In my opinion, the peeked just should hold the next. And every poll_next, it will be changed to new next. Why do *self.as_mut().peeked() = None;

yoshuawuyts

yoshuawuyts reviewed

Oct 28, 2019

View reviewed changes

src/stream/stream/peekable.rs Outdated

S: Stream,

{

pin_utils::unsafe_pinned!(stream: S);

pin_utils::unsafe_unpinned!(peeked: Option<Option<S::Item>>);

Copy link

Contributor

@yoshuawuyts yoshuawuyts Oct 28, 2019

Choose a reason for hiding this comment

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

It'd be nice if this could be updated to use pin-project-lite the way we've done for other combinators (we merged this in the last week; in general it should be nicer!)

@yoshuawuyts

Copy link

Contributor

yoshuawuyts commented Oct 28, 2019

@starsheriff ping; just checking in if there's anything we can do to help unblock you. I'd very much would like to see this PR land!

@yoshuawuyts yoshuawuyts added the enhancement label

Oct 28, 2019

@starsheriff

Copy link

Contributor Author

starsheriff commented Oct 28, 2019

@starsheriff ping; just checking in if there's anything we can do to help unblock you. I'd very much would like to see this PR land!

The comments I got helped a lot already. Haven't had the time the last days to work on it but will pick it up again now. I will ask if I need more help.

starsheriff added 2 commits

October 29, 2019 11:53

@starsheriff


 using next as peekable value

bd104c2

@starsheriff


 tidy up

dc04fde

starsheriff

starsheriff commented

Oct 29, 2019

View reviewed changes

src/stream/stream/peekable.rs

S: Sized,

{

stream: S,

peeked: Option<Poll<Option<S::Item>>>,

Copy link

Contributor Author

@starsheriff starsheriff Oct 29, 2019

Choose a reason for hiding this comment

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

this is what I landed on as the value for peeked

@starsheriff


 fix types

f37916c

starsheriff

starsheriff commented

Oct 29, 2019

View reviewed changes

src/stream/stream/peekable.rs

match &self.peeked {

Some(peeked) => &peeked,

None => {

// how to get the next `next` value? What about `Context`

Copy link

Contributor Author

@starsheriff starsheriff Oct 29, 2019

Choose a reason for hiding this comment

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

this is where I currently have troubles @yoshuawuyts , I have no idea how to get the next next value from here. Given that I store a Option<Poll<Option<S::Item>>> in the struct.

@yoshuawuyts yoshuawuyts mentioned this pull request

Nov 15, 2019

[tracking] streams #129

Open

87 tasks

@yjhmelody

Copy link

Contributor

yjhmelody commented Nov 18, 2019

@starsheriff The peek seem to be diffcult to impl. And if things are async, I think the peek is not very
useful.

Labels

enhancement

3 participants

@starsheriff @yoshuawuyts @yjhmelody

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

add stream::peekable #366

Are you sure you want to change the base?

add stream::peekable #366

Uh oh!

Conversation

@starsheriff starsheriff commented Oct 17, 2019 •

edited

Loading

Uh oh!

Uh oh!

@yjhmelody yjhmelody Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

@yjhmelody yjhmelody Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

@yjhmelody yjhmelody Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

@yoshuawuyts yoshuawuyts Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

yoshuawuyts commented Oct 28, 2019

Uh oh!

starsheriff commented Oct 28, 2019

Uh oh!

@starsheriff starsheriff Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

@starsheriff starsheriff Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

yjhmelody commented Nov 18, 2019

Uh oh!

Uh oh!

add stream::peekable #366

Are you sure you want to change the base?

add stream::peekable #366

Uh oh!

Conversation

@starsheriff starsheriff commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@yjhmelody yjhmelody Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

@yjhmelody yjhmelody Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

@yjhmelody yjhmelody Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

@yoshuawuyts yoshuawuyts Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

yoshuawuyts commented Oct 28, 2019

Uh oh!

starsheriff commented Oct 28, 2019

Uh oh!

@starsheriff starsheriff Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

@starsheriff starsheriff Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

yjhmelody commented Nov 18, 2019

Uh oh!

Uh oh!

@starsheriff starsheriff commented Oct 17, 2019 •

edited

Loading