-
Notifications
You must be signed in to change notification settings - Fork 174
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
impl Stream on Subscription and tweak built-in next() method to align #601
Conversation
types/src/client.rs
Outdated
type Item = Result<Notif, Error>; | ||
fn poll_next(mut self: Pin<&mut Self>, cx: &mut task::Context<'_>) -> task::Poll<Option<Self::Item>> { | ||
let n = futures_util::ready!(self.notifs_rx.poll_next_unpin(cx)); | ||
let res = n.and_then(|n| match serde_json::from_value::<NotifResponse<Notif>>(n) { |
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.
does map
work here?
I don't mind but clippy doesn't like and_then
here...
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.
Ooh good point! I thought I needed and_then
but then didn't realise that I then surrounded all of the result arms in Some
:)
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.
Nice, looks pretty clean too!
Adds a
Stream
impl on theSubscription
type, and tweaks the type signatuer of the built-innext()
method to be identical (and make it use the Stream impl).Really, the only reason to have a built-in
next()
method now is to avoid needing to importStreamExt
, but I think that's a perfectly good reason to keep it, and if the type sig is identical it hopefully won't lead to much confusion :)