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
Introduce stream::select_with_strategy #2450
Conversation
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.
6e4df4e
to
b5bd8fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
55906db
to
2df5523
Compare
2df5523
to
864c69a
Compare
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 } } }`
864c69a
to
e8a17f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @najamelan!
#[pin] | ||
stream2: Fuse<St2>, | ||
state: State, | ||
clos: Clos, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Published in 0.3.16. |
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
whichtakes a closure that defines the selection strategy.
does have 4 generic parameters of which one is a closure, compared to
the 2 streams in
select
. We could also reimplementselect
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....
select_biased
usedelsewhere 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.
in the docs get tested by doc test.