add stream::min_by method #146

montekki · 2019-09-05T15:23:17Z

Implements Stream::min_by.

Not sure if:

Should live in a separate file, but it looks like adding all those methods to stream.rs will bloat at some point.
It can be effectively reused for Stream::min
Other min* and max* should be added to this PR.

yoshuawuyts · 2019-09-05T17:03:37Z

Stdlib: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.min_by

Ref: #129

I agree. We should do this in a separate PR probably though.
Yay!
Yayay! Though doing these PRs one at the time is also welcome; it allows us to review and land them at a faster pace which might be nice for everyone!

src/stream/stream.rs

yoshuawuyts · 2019-09-05T17:20:15Z

src/stream/min_by.rs

+
+impl<S, F> Future for MinBy<S, F>
+where
+ S: futures_core::stream::Stream + Unpin,


Ah, this bound may need to be updated to just be Stream as #140 was merged.

hmmm, you probably mean #145 ?

Ah yes indeed

Is this still unresolved?

yoshuawuyts

Few nits, but overall this looks great! -- thanks so much!

Co-Authored-By: Yoshua Wuyts <yoshuawuyts+github@gmail.com>

montekki · 2019-09-05T18:42:35Z

1. I agree. We should do this in a separate PR probably though.

ok

2. Yay!

Those were questions prefixed by "Not sure if" actually :) So at the point I have no idea about this point

3. Yayay! Though doing these PRs one at the time is also welcome; it allows us to review and land them at a faster pace which might be nice for everyone!

yep, sure

yoshuawuyts

Thanks heaps!

ghost · 2019-09-06T18:41:43Z

src/stream/stream.rs

+ /// ```
+ fn min_by<F>(self, compare: F) -> MinBy<Self, F>
+ where
+ Self: Sized + Unpin,


The Unpin bound here is not necessary because we're taking ownership of self in this method.

ghost · 2019-09-06T18:43:14Z

src/stream/min_by.rs

+
+/// A future that yields the minimum item in a stream by a given comparison function.
+#[derive(Clone, Debug)]
+pub struct MinBy<S: Stream, F> {


Since MinBy is here in a separate file, it'd be a good idea to move Take and others into their own files, too.

But we can do that in a follow-up PR :)

ghost · 2019-09-06T18:46:41Z

src/stream/min_by.rs

+
+impl<S: Stream + Unpin, F> Unpin for MinBy<S, F> {}
+
+impl<S: Stream + Unpin, F> MinBy<S, F> {


Instead of requiring Unpin, we should use unsafe_pinner like for the Take struct.

ghost · 2019-09-06T18:48:40Z

src/stream/min_by.rs

+
+ match next {
+ Some(new) => {
+ cx.waker().wake_by_ref();


Is this line needed at all? I believe it can be deleted.

ghost · 2019-09-06T18:51:35Z

src/stream/min_by.rs

+
+/// A future that yields the minimum item in a stream by a given comparison function.
+#[derive(Clone, Debug)]
+pub struct MinBy<S: Stream, F> {


Since this is a future, it should be named MinByFuture just like AllFuture and AnyFuture.

ghost · 2019-09-06T18:52:46Z

src/stream/stream.rs

+ /// #
+ /// # }) }
+ /// ```
+ fn min_by<F>(self, compare: F) -> MinBy<Self, F>


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.

How is ret! supposed to work for

Futures that take self instead of &mut self and thus should have no lifetime parameters

Streams
?

ghost · 2019-09-06T18:53:36Z

src/stream/stream.rs


 use cfg_if::cfg_if;

+use super::min_by::MinBy;


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).

This reverts commit 7e3599a.

ghost · 2019-09-06T18:56:27Z

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?

montekki · 2019-09-06T19:06:45Z

yep, sure, Ok to do it in a followup to the #150?

ghost · 2019-09-06T20:02:48Z

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 <yoshuawuyts@gmail.com> Co-authored-by: Stjepan Glavina <stjepang@gmail.com>

add stream::min_by method

708a7a6

yoshuawuyts reviewed Sep 5, 2019

View reviewed changes

src/stream/stream.rs Outdated Show resolved Hide resolved

yoshuawuyts reviewed Sep 5, 2019

View reviewed changes

yoshuawuyts approved these changes Sep 5, 2019

View reviewed changes

Update src/stream/stream.rs

fed13fe

Co-Authored-By: Yoshua Wuyts <yoshuawuyts+github@gmail.com>

yoshuawuyts approved these changes Sep 6, 2019

View reviewed changes

yoshuawuyts merged commit 7e3599a into async-rs:master Sep 6, 2019

yoshuawuyts mentioned this pull request Sep 6, 2019

split stream into multiple files #150

Merged

ghost reviewed Sep 6, 2019

View reviewed changes

src/stream/min_by.rs

match next {

Some(new) => {

cx.waker().wake_by_ref();

Copy link

ghost Sep 6, 2019

Choose a reason for hiding this comment

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

Is this line needed at all? I believe it can be deleted.

ghost reviewed Sep 6, 2019

View reviewed changes

ghost pushed a commit that referenced this pull request Sep 6, 2019

Revert "add stream::min_by method (#146)"

0d5f540

This reverts commit 7e3599a.

yoshuawuyts mentioned this pull request Sep 10, 2019

fixes to stream::min_by #162

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

add stream::min_by method #146

add stream::min_by method #146

Uh oh!

montekki commented Sep 5, 2019

yoshuawuyts commented Sep 5, 2019

Uh oh!

yoshuawuyts Sep 5, 2019

montekki Sep 5, 2019

yoshuawuyts Sep 5, 2019

ghost Sep 6, 2019

yoshuawuyts left a comment

montekki commented Sep 5, 2019

yoshuawuyts left a comment

ghost Sep 6, 2019

ghost Sep 6, 2019

ghost Sep 6, 2019

yoshuawuyts Sep 6, 2019

ghost Sep 6, 2019

ghost Sep 6, 2019

ghost Sep 6, 2019

ghost Sep 6, 2019

montekki Sep 6, 2019

ghost Sep 6, 2019

ghost commented Sep 6, 2019

montekki commented Sep 6, 2019

ghost commented Sep 6, 2019

Labels

2 participants


		impl<S: Stream + Unpin, F> Unpin for MinBy<S, F> {}

		impl<S: Stream + Unpin, F> MinBy<S, F> {

add stream::min_by method #146

add stream::min_by method #146

Uh oh!

Conversation

montekki commented Sep 5, 2019

yoshuawuyts commented Sep 5, 2019

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoshuawuyts left a comment

Choose a reason for hiding this comment

montekki commented Sep 5, 2019

yoshuawuyts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 6, 2019

montekki commented Sep 6, 2019

ghost commented Sep 6, 2019

Labels

2 participants