Skip to content

Conversation

@RustyNova016
Copy link
Contributor

@RustyNova016 RustyNova016 commented Feb 28, 2025

Added a function that allows merging two streams using a custom "round robin" policy.

This is an useful alternative to chain as the second stream doesn't wait for the first to finish, and both can be interweaved together

IDK what's the MSRV policy, but that would bump it to 1.82.0. I can bump it down to 1.63.0 but not sure if it's worth it

I'm also trying to upstream it to futures but since the release cycle is sluggish, it can put put here for now
rust-lang/futures-rs#2930

@extremeandy extremeandy self-requested a review March 1, 2025 00:16
/// ```
/// # futures::executor::block_on(async {
/// use futures::stream::{self, StreamExt};
/// use streamies::Streamies as _;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
/// use streamies::Streamies as _;
/// use streamtools::StreamTools as _;


// Check if we should be polling from the first stream.
// This means:
// - It's our turn to be polled AND the se
Copy link
Owner

Choose a reason for hiding this comment

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

Incomplete comment

// - The stream isn't ended
if this.first_count < &mut this.first_nb_ele.get() {
if let Some(first) = this.first.as_mut().as_pin_mut() {
if let Some(item) = ready!(first.poll_next(cx)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Is this behaviour intentional - to wait for the first stream to yield, even if there are items available on the second stream?

My intuition would have been to fall back to the other stream if the first is pending, but I guess it depends on the use case.

Could possibly make this parameterisable or have a different variant which has the fallback behaviour.

Copy link
Owner

Choose a reason for hiding this comment

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

@RustyNova016 any thoughts on this? I think it'd be nice to have two variants: merge_round_robin and merge_round_robin_strict.

use pin_project_lite::pin_project;

pin_project! {
/// Stream for the [`merge_round_robin`](crate::Streamies::merge_round_robin) method.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
/// Stream for the [`merge_round_robin`](crate::Streamies::merge_round_robin) method.
/// Stream for the [`merge_round_robin`](crate::StreamTools::merge_round_robin) method.

Comment on lines +44 to +47
"Couldn't convert `first_nb_ele` to `NonZeroUsize`. The value must no be 0",
),
second_nb_ele: NonZeroUsize::new(second_nb_ele).expect(
"Couldn't convert `second_nb_ele` to `NonZeroUsize`. The value must no be 0",
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
"Couldn't convert `first_nb_ele` to `NonZeroUsize`. The value must no be 0",
),
second_nb_ele: NonZeroUsize::new(second_nb_ele).expect(
"Couldn't convert `second_nb_ele` to `NonZeroUsize`. The value must no be 0",
"Couldn't convert `first_nb_ele` to `NonZeroUsize`. The value must not be 0",
),
second_nb_ele: NonZeroUsize::new(second_nb_ele).expect(
"Couldn't convert `second_nb_ele` to `NonZeroUsize`. The value must not be 0",

///
/// The resulting stream emits `nb_self` elements from the first stream,
/// then `nb_other` from the other. When one of the stream finishes, the
/// second is then used.
Copy link
Owner

Choose a reason for hiding this comment

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

Could you also add a comment explaining that nb_self and nb_other must be non-zero or the method will panic?

@extremeandy
Copy link
Owner

IDK what's the MSRV policy, but that would bump it to 1.82.0. I can bump it down to 1.63.0 but not sure if it's worth it

IDK either 😅 I'll leave it to you.

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.

2 participants