add stream::min_by method #146
Conversation
|
Stdlib: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.min_by Ref: #129
|
Co-Authored-By: Yoshua Wuyts <[email protected]>
ok
Those were questions prefixed by "Not sure if" actually :) So at the point I have no idea about this point
yep, sure |
| /// ``` | ||
| fn min_by<F>(self, compare: F) -> MinBy<Self, F> | ||
| where | ||
| Self: Sized + Unpin, |
There was a problem hiding this comment.
The Unpin bound here is not necessary because we're taking ownership of self in this method.
|
|
||
| /// A future that yields the minimum item in a stream by a given comparison function. | ||
| #[derive(Clone, Debug)] | ||
| pub struct MinBy<S: Stream, F> { |
There was a problem hiding this comment.
Since MinBy is here in a separate file, it'd be a good idea to move Take and others into their own files, too.
|
|
||
| impl<S: Stream + Unpin, F> Unpin for MinBy<S, F> {} | ||
|
|
||
| impl<S: Stream + Unpin, F> MinBy<S, F> { |
There was a problem hiding this comment.
Instead of requiring Unpin, we should use unsafe_pinner like for the Take struct.
|
|
||
| match next { | ||
| Some(new) => { | ||
| cx.waker().wake_by_ref(); |
There was a problem hiding this comment.
Is this line needed at all? I believe it can be deleted.
|
|
||
| /// A future that yields the minimum item in a stream by a given comparison function. | ||
| #[derive(Clone, Debug)] | ||
| pub struct MinBy<S: Stream, F> { |
There was a problem hiding this comment.
Since this is a future, it should be named MinByFuture just like AllFuture and AnyFuture.
| /// # | ||
| /// # }) } | ||
| /// ``` | ||
| fn min_by<F>(self, compare: F) -> MinBy<Self, F> |
There was a problem hiding this comment.
The return type here should use ret! like in fn any, for example. This function would ideally be an async fn if Rust supported that syntax in traits today.
There was a problem hiding this comment.
How is ret! supposed to work for
- Futures that take
selfinstead of&mut selfand thus should have no lifetime parameters - Streams
?
| use std::pin::Pin; | ||
|
|
||
| use cfg_if::cfg_if; | ||
|
|
||
| use super::min_by::MinBy; |
There was a problem hiding this comment.
Since min_by should ideally be an async fn, let's make MinBy a hidden type and not even implement Debug (because async fn desugaring doesn't implement Debug for its future).
|
Sorry for taking so long to review this! :( Since there's no way to revert and re-open this PR, could you please address my comment in a follow-up PR? |
|
yep, sure, Ok to do it in a followup to the #150? |
|
yeah, sounds good! :) |
149: update deps r=stjepang a=yoshuawuyts Updates all deps. Thanks! 150: split stream into multiple files r=stjepang a=yoshuawuyts This splits `stream/mod.rs`'s combinators into multiple files, making it easier to contribute combinators. Additionally we've renamed `MinBy` to `MinByFuture` to make it the same as the other private futures. Ref #146 #129. Thanks! Co-authored-by: Yoshua Wuyts <[email protected]> Co-authored-by: Stjepan Glavina <[email protected]>
Implements
Stream::min_by.Not sure if:
stream.rswill bloat at some point.Stream::minmin*andmax*should be added to this PR.