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

Add enumerate combinator to Stream #832

Merged
merged 3 commits into from
Jan 24, 2019
Merged

Add enumerate combinator to Stream #832

merged 3 commits into from
Jan 24, 2019

Conversation

zaharidichev
Copy link
Contributor

Motivation

Motivation is to provide an enumarete combinator, much like in Iterator. The issue for that can be found here: #741

Solution

Simply provided Stream::enumerate on StreamExt

Since this is my first PR any kind of feedback will be greatly appreaciated :)

@NeoLegends
Copy link
Contributor

NeoLegends commented Jan 6, 2019

Sorry for my comment here since I'm not a maintainer of any kind, but I think this belongs into the futures-rs repository, since it doesn't need anything of the tokio runtime to work, does it? :)

see https://github.com/rust-lang-nursery/futures-rs

@zaharidichev
Copy link
Contributor Author

@NeoLegends the issue is in this repository, so I thought that it can go here. Also, there is StreamExt that I thought has the purpose of adding such additional functionality on top of the Stream coming from futures-rs. Correct me if I am wrong...

@davidbarsky
Copy link
Member

@zaharidichev Given that futures-rs is primarily targeted at Futures 0.3/std and Tokio isn't, I think Tokio is a good place to open a PR, but @carllerche can correct me.

Copy link
Member

@davidbarsky davidbarsky left a comment

Choose a reason for hiding this comment

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

These changes look good to me, but I wonder if it makes sense to have a corresponding Sink implementation in the same way that Fuse.

@zaharidichev
Copy link
Contributor Author

@davidbarsky The reason I did not put a forwarding sink impl is because I saw that throttle does not have one. Not sure whether this is a valid reason but in any case it would be helpful if somebody explains why throttle does not have a Sink and whether enumarete should have one :)

@davidbarsky
Copy link
Member

Accident of history, as far as I can tell, but others can more clearly explain. Enumerate is closer to the “iterator-like” combinators than throttle. Since the iterator-like combinators tend to have Sink implementations (skip_while, chunks). I think that because of the more iterator-like features of enumerate, it should also have a corresponding Sink interface.

@zaharidichev
Copy link
Contributor Author

thanks for the explanation @davidbarsky. I have added a sink impl

@davidbarsky
Copy link
Member

Thanks @zaharidichev! I think this looks good. I can merge if build succeeds, @carllerche?

@zaharidichev
Copy link
Contributor Author

Thanks @davidbarsky Since I am a bit new to this repo, is there some other biginner friendly ticket I can pick up ?

@davidbarsky
Copy link
Member

@zaharidichev Anything under the label “Help Wanted”. In my opinion, the biggest return on effort (and the best way to go from “beginner” to “expert” extremely quickly) is documentation. Issues are tracked at tokio-rs/doc-push, and picking one of those issues up and opening a PR on https://github.com/tokio-rs/website would be extremely helpful.

@zaharidichev
Copy link
Contributor Author

Yes, documentation sounds like a good way to self educate. I will pick something up from there :) Thanks !

@zaharidichev
Copy link
Contributor Author

I wonder why the build is showing as in progress when it actually failed on stable ?

@LucioFranco
Copy link
Member

@zaharidichev seems like the travis forgot to update the commit but it seems that try_ready! isn't in scope. https://travis-ci.org/tokio-rs/tokio/jobs/476408795#L672

@zaharidichev
Copy link
Contributor Author

Yes @LucioFranco, sorry about that. It passes now.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Overall this LGTM 👍

src/util/enumerate.rs Show resolved Hide resolved
src/util/enumerate.rs Outdated Show resolved Hide resolved
src/util/enumerate.rs Outdated Show resolved Hide resolved
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Just some minor requests inline.

@@ -9,7 +9,7 @@ use futures::Stream;

#[cfg(feature = "timer")]
use std::time::Duration;

use util::enumerate::Enumerate;
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to be a pub export.

}

impl<T> Enumerate<T> {
pub fn new(stream: T) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid making this public? Construction can happen via the stream trait.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, not entirely sure how that works. Can you give me a clue. Essentially we need to make it module private but how does that tie in with the pub export that you mentioned earlier ?

Copy link
Member

Choose a reason for hiding this comment

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

pub(crate) fn new(...) then call Enumerate::new in `StreamExt::enumerate().

src/util/enumerate.rs Show resolved Hide resolved
@zaharidichev
Copy link
Contributor Author

@carllerche I think I have addressed your comments. Is this good to merge now ?

@carllerche
Copy link
Member

@zaharidichev Just one missing pub here then it should be good. (the use in futures.rs should be pub so that Enumerate can be named via tokio::util::futures::Enumerate.

@zaharidichev
Copy link
Contributor Author

@carllerche Can that be merged now ?

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

👍

@carllerche carllerche merged commit fbad629 into tokio-rs:master Jan 24, 2019
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