Conversation
(fwiw, for this use case in particular, i really should work on upstreaming the I think this is probably worth having for the general case, though it's kind of hard to explain why you would want it until you already understand why you would want it...not totally sold on the naming but I guess it's probably the best option... |
Perhaps 😅
Yes agreed. Writing the docs was also a bit tricky without involving I'm not sold on the name either. I also considered Regardless, I've cleaned things up so ready for review 👀 |
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
this looks good to me! i had a few thoughts...
| impl<R, S, F, T, E, Fut> Service<R> for MapFuture<S, F> | ||
| where | ||
| S: Service<R>, | ||
| F: FnMut(S::Future) -> Fut, | ||
| E: From<S::Error>, | ||
| Fut: Future<Output = Result<T, E>>, | ||
| { | ||
| type Response = T; | ||
| type Error = E; |
There was a problem hiding this comment.
TIOLI: we could reduce the number of type parameters here using TryFuture:
| impl<R, S, F, T, E, Fut> Service<R> for MapFuture<S, F> | |
| where | |
| S: Service<R>, | |
| F: FnMut(S::Future) -> Fut, | |
| E: From<S::Error>, | |
| Fut: Future<Output = Result<T, E>>, | |
| { | |
| type Response = T; | |
| type Error = E; | |
| impl<R, S, F, Fut> Service<R> for MapFuture<S, F> | |
| where | |
| S: Service<R>, | |
| F: FnMut(S::Future) -> Fut, | |
| Fut: TryFuture, | |
| Fut::Error: From<S::Error>, | |
| { | |
| type Response = Fut::Ok; | |
| type Error = Fut::Error; |
not a big deal though...
There was a problem hiding this comment.
Interesting! How does this work in terms of not exposing things from the futures crate?
I ran into a thing today where I wanted to write a middleware that wraps all futures produced by a service in a `tracing::Span`. So something like `self.inner.call(req).instrument(span)`. Afaik all the combinators we have today receive the value produced by the future and not the future itself. At that point its too late to call `.instrument`. So I thought this made sense to add a combinator for.
davidpdrsn
force-pushed
the
david/map-future
branch
from
5c15ea7 to
835b9fb
Compare
I forgot at add this one in #542. Think its nice to have for consistency.
I forgot at add this one in #542. Think its nice to have for consistency.
* builder: Add `ServiceBuilder::map_future` I forgot at add this one in #542. Think its nice to have for consistency. * Update tower/src/builder/mod.rs Co-authored-by: Eliza Weisman <[email protected]> Co-authored-by: Eliza Weisman <[email protected]>
2 participants
I ran into a thing today where I wanted to write a middleware that wraps
all futures produced by a service in a
tracing::Span. So somethinglike
self.inner.call(req).instrument(span).Afaik all the combinators we have today receive the value produced by
the future and not the future itself. At that point its too late to call
.instrument. So I thought this made sense to add a combinator for.If you agree I'll go write some docs and clean things up.