Skip to content
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

change NewService to MakeService<Target, Request> #114

Merged
merged 1 commit into from
Nov 8, 2018

Conversation

seanmonstar
Copy link
Collaborator

Builds off the generic-request branch.

cc #108

@seanmonstar
Copy link
Collaborator Author

This also changed NewServiceFn to just ServiceFn, since you can get a MakeService by having a ServiceFn that returns a Service.

One place where this was weird was with Reconnect, where it now has ResponseFuture<T, Ctx, Request>, even though it doesn't really need those generics otherwise...

where
T: NewService<Request>,
T: MakeService<Ctx, Request>,
Ctx: Clone,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking that instead of requiring Clone here, it'd be more flexible if the bounds were T: MakeService<&Ctx, Request>. Then, if the new Service needs a clone, it can make one, but it's otherwise not wasted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is worth a shot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did that pan out?

@jonhoo
Copy link
Collaborator

jonhoo commented Oct 25, 2018

I'd like to see something in the docs about what Ctx is, what it's intended for, etc.

@seanmonstar
Copy link
Collaborator Author

I explored if something like this could be used in hyper (allowing user's to grab the remote address) (and while not being a breaking change yet), and it works: hyperium/hyper@94cc806

Though I had to some nasty crap to get S: for<'a> MakeService<&'a Io> to work...

Copy link
Member

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

I only had a bike shed level comment. Also, you had an open question regarding Reconnect, but that can always be done later.

{
new_service: T,
state: State<T, Request>,
context: Ctx,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are bike shedding, I prefer "target" over "context". I feel that it makes the intent a bit more clear.

@@ -23,47 +24,47 @@ pub enum Error<T, U> {
NotReady,
}

pub struct ResponseFuture<T, Request>
pub struct ResponseFuture<T, Ctx, Request>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it is an unrelated change... but can this just be ResponseFuture<T> where T is the inner future?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not easily, since the error is mapped by Error::Inner which means the bounds are needed. Perhaps with some better refactoring...

where
T: NewService<Request>,
T: MakeService<Ctx, Request>,
Ctx: Clone,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did that pan out?

@carllerche
Copy link
Member

Can we 🚢 🇮🇹 ?

@jonhoo
Copy link
Collaborator

jonhoo commented Nov 7, 2018

Note to self: this affects #119.

@seanmonstar seanmonstar changed the base branch from generic-request to master November 8, 2018 23:01
@seanmonstar seanmonstar changed the title change NewService to MakeService<Ctx, Request> change NewService to MakeService<Target, Request> Nov 8, 2018
@seanmonstar seanmonstar merged commit 373d017 into master Nov 8, 2018
@seanmonstar seanmonstar deleted the new-service-arg branch November 8, 2018 23:12
@jonhoo
Copy link
Collaborator

jonhoo commented Nov 9, 2018

I'd still like to see some docs for what the Target param is intended for. Is it reasonable to make it ()? Why/why not? For #119, I have a thing that makes new services on demand; how can it generate appropriate Target values for anything but ()?

jonhoo added a commit to jonhoo/tower that referenced this pull request Nov 9, 2018
This currently uses `Target=()`, because I don't know how `Target` is
supposed to be used.
hawkw added a commit to hawkw/tokio-trace-prototype that referenced this pull request Nov 9, 2018
This is caused by tower-rs/tower#114, which some tower crates haven't picked
up yet. It's necessary to specify a `[replace]` since `[patch]` can't override
dependencies with the same source.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit to hawkw/tokio-trace-prototype that referenced this pull request Nov 15, 2018
This updates the `tokio-trace-tower` and `tokio-trace-tower-http`
compatibility crates to be compatible with tower-rs/tower#114 and
tower-rs/tower-h2#42.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit to hawkw/tokio-trace-prototype that referenced this pull request Nov 15, 2018
This updates the `tokio-trace-tower` and `tokio-trace-tower-http`
compatibility crates to be compatible with tower-rs/tower#114 and
tower-rs/tower-h2#42.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
jonhoo added a commit to jonhoo/tower that referenced this pull request Nov 19, 2018
This currently uses `Target=()`, because I don't know how `Target` is
supposed to be used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants