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

Introduce stream::select_with_strategy #2450

Merged
merged 7 commits into from Jun 17, 2021

Conversation

najamelan
Copy link
Contributor

@najamelan najamelan commented Jun 8, 2021

The current stream::select function only supports round robin. To change that
would be a breaking change, so for now I propose a select_bias which
takes a closure that defines the selection strategy.

  • When a breaking change is planned, we could re-consider this, but it
    does have 4 generic parameters of which one is a closure, compared to
    the 2 streams in select. We could also reimplement select on top of this,
    we just need to deal with hiding the generics of the closure, but I reckon a
    function pointer can be named and we don't need to capture anything....
  • note the name select_bias. It's not consistent with select_biased used
    elsewhere in the lib. This can obviously be changed, but I feel the "ed"
    at the end does not add any semantic value but makes function names
    longer.
  • There isn't really integration tests included right now, but the examples
    in the docs get tested by doc test.

@najamelan najamelan requested a review from taiki-e as a code owner Jun 8, 2021
The current select function only supports round robin. To change that
would be a breaking change, so for now I propose a `select_bias` which
takes a closure that defines the selection strategy.

- When a breaking change is planned, we could re-consider this, but it
  does have 4 generic parameters of which one is a closure, compared to
  the 2 streams in `select`.
- note the name select_bias. It's not consistent with `select_biased` used
  elsewhere in the lib. This can obviously be changed, but I feel the "ed"
  at the end does not add any semantic value but makes function names
  longer.
- There isn't really integration tests included right now, but the examples
  in the docs get tested by doc test.
@najamelan najamelan force-pushed the feature/stream_select_bias branch from 6e4df4e to b5bd8fe Compare Jun 8, 2021
Copy link
Member

@taiki-e taiki-e left a comment

Thanks for the PR! The implementation looks good to me.

  • I feel a name like select_with_strategy would be more good because it is the user who decides whether this is biased or not.
  • Is it possible to use this as an internal implementation of select? (without breaking change)

futures-util/src/stream/select_bias.rs Outdated Show resolved Hide resolved
futures-util/src/stream/select_bias.rs Outdated Show resolved Hide resolved
futures-util/src/stream/select_bias.rs Outdated Show resolved Hide resolved
futures-util/src/stream/select_bias.rs Outdated Show resolved Hide resolved
futures-util/src/stream/select_bias.rs Outdated Show resolved Hide resolved
futures-util/src/stream/select_bias.rs Outdated Show resolved Hide resolved
futures-util/src/stream/select_bias.rs Outdated Show resolved Hide resolved
futures-util/src/stream/select_bias.rs Outdated Show resolved Hide resolved
futures-util/src/stream/select_bias.rs Outdated Show resolved Hide resolved
futures-util/src/stream/select_bias.rs Outdated Show resolved Hide resolved
@taiki-e taiki-e added A-stream S-waiting-on-author labels Jun 16, 2021
@taiki-e taiki-e changed the title Introduce futures_util::stream::select_bias. Introduce stream::select_with_strategy Jun 16, 2021
futures-util/src/stream/select.rs Outdated Show resolved Hide resolved
futures-util/src/stream/select.rs Outdated Show resolved Hide resolved
@najamelan najamelan force-pushed the feature/stream_select_bias branch from 55906db to 2df5523 Compare Jun 17, 2021
@najamelan najamelan force-pushed the feature/stream_select_bias branch from 2df5523 to 864c69a Compare Jun 17, 2021
This allows not to print the closure and give `Select` a derive impl.

The result for `Select` looks like:

`Select { inner: SelectWithStrategy { stream1: Fuse { stream: Repeat { item: 1 }, done: false }, stream2: Fuse { stream: Repeat { item: 2 }, done: false } } }`
@najamelan najamelan force-pushed the feature/stream_select_bias branch from 864c69a to e8a17f9 Compare Jun 17, 2021
Copy link
Member

@taiki-e taiki-e left a comment

LGTM. Thanks @najamelan!

@taiki-e taiki-e merged commit cb267a7 into rust-lang:master Jun 17, 2021
22 checks passed
@taiki-e taiki-e added 0.3-backport: pending and removed S-waiting-on-author labels Jun 17, 2021
#[pin]
stream2: Fuse<St2>,
state: State,
clos: Clos,
Copy link
Contributor

@pickfire pickfire Jun 28, 2021

Choose a reason for hiding this comment

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

Is this a typo for close?

Copy link
Contributor Author

@najamelan najamelan Jun 28, 2021

Choose a reason for hiding this comment

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

It's the generic representing the closure. So not a typo. Of course another name can be chosen. I don't really care about what it's called.

@taiki-e taiki-e mentioned this pull request Jul 23, 2021
@taiki-e taiki-e added 0.3-backport: completed and removed 0.3-backport: pending labels Jul 23, 2021
@taiki-e taiki-e mentioned this pull request Jul 23, 2021
@taiki-e
Copy link
Member

taiki-e commented Jul 23, 2021

Published in 0.3.16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants