-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
dev: add send_and_resolve #10847
dev: add send_and_resolve #10847
Conversation
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.
one suggestion
type MaybePayloadFuture<P> = | ||
Pin<Box<dyn Future<Output = Option<Result<P, PayloadBuilderError>>> + Send + Sync>>; |
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.
I don't think we need this
@@ -127,6 +129,31 @@ where | |||
} | |||
} | |||
|
|||
/// Sends a message to the service to start building a new payload for the given payload | |||
/// attributes and returns a future that resolves to the payload. | |||
pub fn send_and_resolve_payload( |
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.
this can just be an async function
let id = match rx.await { | ||
Err(e) => return Some(Err(e.into())), | ||
Ok(Err(e)) => return Some(Err(e)), | ||
Ok(Ok(id)) => id, | ||
}; | ||
|
||
let (tx, rx) = oneshot::channel(); | ||
to_service.send(PayloadServiceCommand::Resolve(id, tx)).ok()?; | ||
match rx.await.transpose()? { | ||
Ok(fut) => Some(fut.await), | ||
Err(e) => Some(Err(e.into())), | ||
} |
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.
then you can do all of this without the Box::pin
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.
Ah ok I thought you wanted to return a future, will update
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.
yeah but you can return Option<PayloadFuture>
or result PayloadFuture, PayloadNotfound
e6f3e1d
to
15eb510
Compare
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.
lgtm
Adds a send and resolve helper on the
PayloadBuilderHandle
. This helper allows to request for a payload to be build based on the provided payload builder attribute and returns a future that resolves to the payload.Resolves #10845