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

v2.4.0 #102

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

v2.4.0 #102

wants to merge 1 commit into from

Conversation

notgull
Copy link
Member

@notgull notgull commented May 26, 2024

@taiki-e
Copy link
Collaborator

taiki-e commented Jun 1, 2024

  • a "take_until" method
  • It seems that what was actually added was an freestanding function (stream::take_until) not a trait method (StreamExt::take_until)? (The PR title and commit message also says StreamExt::take_until)
  • The Iterator::take_while takes FnMut(&Self::Item) -> bool as a predicate. I don't remember if futures had the same method but maybe it takes Fn* which returns a future. Is there any reason why ours decided to take a future compared to those?

@notgull
Copy link
Member Author

notgull commented Jun 1, 2024

Good catch! I forgot to check the equivalent Iterator method, I'll fix.

@notgull
Copy link
Member Author

notgull commented Jun 1, 2024

Actually, on second check I think it's intentional. The idea is "run a stream in parallel with a future and stop the stream when the future stops." I think the idea is this:

let my_stream = /* ... */;
let (signal, shutdown) = async_channel::bounded::<()>(1);

// Send the shutdown signal when the user hits ctrl-c.
executor.spawn(async move {
    wait_for_user_to_hit_ctrl_c().await;
    signal.try_send(()).ok();
}).detach();

// Run until the stream stops.
while let Some(item) = stream::take_until(my_stream, shutdown).next().await {
    do_something(item).await;
}

@taiki-e
Copy link
Collaborator

taiki-e commented Jun 3, 2024

(Honestly, I have no strong opinion on which API to choose here. I just know what the result of inconsistent APIs can be: rust-lang/futures-rs#2755)

@notgull
Copy link
Member Author

notgull commented Jun 4, 2024

I think it's not an inconsistent API, since it's somewhat Stream-specific. However I would be okay with changing the name to distinguish it from any Iterator functions.

@notgull
Copy link
Member Author

notgull commented Jun 6, 2024

I think the name is the core point of confusion here. I've opened #104 to address this.

@notgull
Copy link
Member Author

notgull commented Jun 8, 2024

I've changed the name,

@notgull
Copy link
Member Author

notgull commented Aug 12, 2024

@smol-rs/admins Can someone take a look at this?

CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Copy link
Member Author

notgull commented Sep 6, 2024

clippy is not happy tho about

Fixed!

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

Successfully merging this pull request may close these issues.

3 participants