-
Notifications
You must be signed in to change notification settings - Fork 11
Conversation
A public named type for the background task future, needed to make the signature of the Service trait implementation of Connect usable with generic executor types. Replaces ConnectExecutor.
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 looks great to me. Two small formatting comments and I'd like to keep the ConnectExecutor
trait. Let me know what you think 😄
d0ad5ff
to
492940f
Compare
As per discussion in tower-rs#46
Respecified and blanket implemented in terms of client::Background. Make it a public type as well.
As per discussion in tower-rs#46
492940f
to
bea987b
Compare
ConnectExecutor is a utility trait to bind generic type parameters as supplied by the API user and expose their bounds required for an executor. Relegating part of it to LiftBody does not make sense.
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.
Very happy with this, one small thing left and I think this is good.
mod connect; | ||
mod connection; | ||
mod future; | ||
|
||
pub use self::connect::{Connect, ConnectError}; | ||
pub use self::background::Background; |
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 believe the type needs to be re-exported. It can be "sealed".
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.
With the public ConnectExecutor
, the only reason to have Background
public is for completeness of documentation.
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.
The downside is it is now part of the public API, meaning changes to the type == breaking change.
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.
It was not published in a crates.io release, so it's not too late for #49 to fix this 😄
A public named type for the background task future, needed to make the signature of the
Service
trait implementation ofConnect
usable with generic executor types.ReplacesTheConnectExecutor
ConnectExecutor
alias trait uses this type as the parameter for itsTypedExecutor
prerequisite and is made public.Fixes #44