Skip to content
This repository has been archived by the owner on Apr 16, 2020. It is now read-only.

Introduce client::Background #46

Merged
merged 5 commits into from
Jun 4, 2019

Conversation

mzabaluev
Copy link
Contributor

@mzabaluev mzabaluev commented Jun 3, 2019

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 The ConnectExecutor alias trait uses this type as the parameter for its TypedExecutor prerequisite and is made public.

Fixes #44

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.
Copy link
Member

@LucioFranco LucioFranco left a 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 😄

src/client/background.rs Outdated Show resolved Hide resolved
src/client/background.rs Outdated Show resolved Hide resolved
src/client/connect.rs Show resolved Hide resolved
src/client/connect.rs Show resolved Hide resolved
@mzabaluev mzabaluev force-pushed the client-background branch from d0ad5ff to 492940f Compare June 4, 2019 06:42
mzabaluev added a commit to mzabaluev/tower-hyper that referenced this pull request Jun 4, 2019
mzabaluev added 2 commits June 4, 2019 09:45
Respecified and blanket implemented in terms of client::Background.
Make it a public type as well.
@mzabaluev mzabaluev force-pushed the client-background branch from 492940f to bea987b Compare June 4, 2019 06:45
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.
Copy link
Member

@LucioFranco LucioFranco left a 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.

src/client/connect.rs Outdated Show resolved Hide resolved
src/client/background.rs Outdated Show resolved Hide resolved
@LucioFranco LucioFranco merged commit 3355a28 into tower-rs:master Jun 4, 2019
@mzabaluev mzabaluev deleted the client-background branch June 4, 2019 17:18
mod connect;
mod connection;
mod future;

pub use self::connect::{Connect, ConnectError};
pub use self::background::Background;
Copy link
Member

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".

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Private trait in Service implementation bounds for Connect
3 participants