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.

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

Refactor debounce implementation #833

Closed
wants to merge 3 commits into from
Closed

Refactor debounce implementation #833

wants to merge 3 commits into from

Conversation

NeoLegends
Copy link
Contributor

@NeoLegends NeoLegends commented Jan 6, 2019 •
edited
Loading

This addresses the feedback from #747 (review).

Improved documentation will follow.

carllerche
carllerche reviewed Jan 6, 2019
View reviewed changes
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.

Thanks.

Unless you need it now, ehat do you think about keeping the builder private initially and only have the StreamExt fns as ways to create them? Then we have more time to refine the Api using real use cases.

Also, could you explain more what debounce is and when it is useful for Tokio apps?

src/lib.rs
@@ -117,6 +117,8 @@ extern crate tokio_uds;
pub mod clock;
#[cfg(feature = "codec")]
pub mod codec;
#[cfg(feature = "timer")]
pub mod debounce;
Copy link
Member

Choose a reason for hiding this comment

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

The module should probably be under tokio::timer.

@carllerche carllerche mentioned this pull request Jan 6, 2019
Merged
@carllerche
Copy link
Member

I'm going to revert the original PR (#834) as it broke CI (due to a conflict w/ another PR i believe) and I need to release crates and we still need to tweak the public API. Once that PR lands, could you update this PR to be a take 2?

Sorry for the churn.

carllerche added a commit that referenced this pull request Jan 7, 2019
@carllerche
Revert "util: implement stream debounce combinator (#747)" (#834)
74c73b2 
This reverts commit 7a49ebb.

The commit conflicted with another change that was merged, causing CI to fail. The public API
also requires a bit more refinement (#833) and Tokio crates need to be released.
@carllerche
Copy link
Member

Thanks for the work 👍 I'm going to close this due to inactivity and stagnation. If you want to revisit, we can talk about what steps are required to update it to the latest changes on master.

@carllerche carllerche closed this Jul 8, 2019
@NeoLegends
Copy link
Contributor Author

So I'm actually in a situation where I'd like to revisit this right now.

Use-case: We receive update events via a Stream and want to forward them via Websockets to browser clients. Because the update rate can be quite high, we want to allow the browsers to apply a rate limit to the updates. The websocket server would then wait a fixed time between sending updates and throw away all the others.

@extremeandy
Copy link

I'm very keen to see sample and debounce and a whole raft of extensions on streams to bring us closer to feature parity with ReactiveX. Any chance of reviving this?

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

@carllerche carllerche carllerche left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@NeoLegends @carllerche @extremeandy