-
Notifications
You must be signed in to change notification settings - Fork 283
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
Add and_then combinator #485
Conversation
@LucioFranco Just in case you missed this discussion: hlb8122#1 |
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.
Overall, this looks good! I had one suggestion around error types.
Also, it would be nice to add a ServiceBuilder
method for pushing an and_then
layer, similar to the ones for other combinators.
ad83154
to
cfd520d
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.
overall, this looks good. let's change the response future type to a newtype.
i also had a small style suggestion, but you can take it or leave it.
tower/src/util/and_then.rs
Outdated
{ | ||
type Response = Fut::Ok; | ||
type Error = Error; | ||
type Future = AndThenFut<ErrIntoFut<S::Future, Error>, Fut, F>; |
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.
let's change this to a named type re-exported from util::future
, the way the other combinators do. I added a macro to make this easier --- if you rebase onto master, you can follow the example of PR #509
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 doesn't seem possible:
opaque_future! {
/// Response future from [`AndThen`] services.
///
/// [`AndThen`]: crate::util::AndThen
pub type AndThenFuture<F1, F2: TryFuture, N> = future::AndThen<future::ErrInto<F1, F2::Error>, F2, N>;
}
Is this acceptable instead?
opaque_future! {
/// Response future from [`AndThen`] services.
///
/// [`AndThen`]: crate::util::AndThen
pub type AndThenFuture<F1, F2, F2Error, N> = future::AndThen<future::ErrInto<F1, F2Error>, F2, N>;
}
EDIT: I could also just write the top one without using the macro.
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.
Either is fine --- you're right that the macro doesn't support trait bounds currently. We could try to hack that in, but I think it would be fine to manually write our own newtype.
Remember that if you do that, you should write pub struct AndThenFuture<...>(...)
and write a Future
impl for it, rather than pub type AndThenFuture...
. This is what the macro would expand to.
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.
How's that look?
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.
looks good to me, thank you!
Motivation
https://docs.rs/futures/0.3.8/futures/future/trait.TryFutureExt.html#method.and_then is a useful method on futures. Perhaps it'd be nice to replicate this for the
ServiceExt
API.TODO