Add util::option_layer and ServiceBuilder::option_layer
#555
Conversation
|
Thanks 👍 Should probably also be mentioned in the changelog. |
| @@ -938,3 +940,12 @@ pub trait ServiceExt<Request>: tower_service::Service<Request> { | |||
| } | |||
|
|
|||
| impl<T: ?Sized, Request> ServiceExt<Request> for T where T: tower_service::Service<Request> {} | |||
|
|
|||
| /// Convert an `Option<Layer>` into a `Layer` | |||
| pub fn option_layer<L>(layer: Option<L>) -> Either<L, Identity> { | |||
There was a problem hiding this comment.
An alternative to having a special wrapper function would be to just add an impl<L: Layer> for Option<L>. That might have slightly nicer API ergonomics, since all Option<Layer>s just are layers, rather than having to wrap them in option_layer calls.
But, the downside to that is that this would have to be implemented in the tower_layer crate, rather than in tower. This means that we couldn't use Either, which is defined in tower...
There was a problem hiding this comment.
I actually like that option_layer is explicit about whats happening. When looking at large layer stack being built with ServiceBuilder it might be obvious which layers are optional and which aren't. Is this TimeoutLayer applied all the time or only sometimes? Would have to check the type.
That combined with the annoyances of having to copy+paste Either (or depend on the either crate) makes me think this approach is fine.
There was a problem hiding this comment.
@davidpdrsn Yeah, I think that makes sense. I'm also in favor of avoiding tower-service changes when they're not absolutely necessary. We can always add an impl in tower-service later if we end up deciding that is a good idea...
There was a problem hiding this comment.
for API consistency's sake, WDYT about adding an option_service (or something) that takes an Option<T: Service> and returns an Optional<T>? That way, we'd have option_layer and option_service functions.
I'd add that in a separate PR, though.
kazk
force-pushed
the
add-option-layer
branch
from
63c95af
to
741eb1f
Compare
kazk
changed the title
util::option_layer and ServiceBuilder::option_layer
|
Thanks for reviewing. Added minimal examples with |
kazk
force-pushed
the
add-option-layer
branch
2 times, most recently
from
9ef34ed
to
9d523bc
Compare
kazk
force-pushed
the
add-option-layer
branch
from
9d523bc
to
4fc5947
Compare
|
Fixed doctests. Sorry for the noise. |
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Closes #553