Add combinators for working with Services by fafhrd91 · Pull Request #104 · tower-rs/tower

fafhrd91 · 2018-09-01T20:50:14Z

ServiceExt trait
and_then, then, map, map_err, from_err, apply combinators

fafhrd91 · 2018-09-01T20:50:22Z

hawkw

This seems right at a glance, so far. I think there's some code here that can be simplified with try_ready!

hawkw · 2018-09-02T00:50:18Z

tower-util/src/ext/then.rs

+    type Future = ThenFuture<A, B>;
+
+    fn poll_ready(&mut self) -> Poll<(), Self::Error> {
+        match self.a.poll_ready() {


Take it or leave it: I think this could just be

let _ = try_ready!(self.a.poll_ready()); self.b.poll_ready()

hawkw · 2018-09-02T00:52:36Z

tower-util/src/ext/map.rs

+    type Error = T::Error;
+
+    fn poll(&mut self) -> Poll<Self::Item, Self::Error> {
+        match self.fut.poll()? {


This could just be

let resp = try_ready!(self.fut.poll()); Ok(Async::Ready((self.f)(resp)))

hawkw · 2018-09-02T00:53:19Z

tower-util/src/ext/apply.rs

+pub struct Apply<T, F, R, Req> {
+    service: T,
+    f: F,
+    r: PhantomData<Fn(Req) -> R>,


Nit: since this field isn't used, it should probably have a leading _.

I also think only the request is needed as a generic on the struct?

i am not sure, we need F which is Fn(Req, T) -> IntoFuture. do i miss anything?

hawkw · 2018-09-02T00:54:15Z

tower-util/src/ext/and_then.rs

+            return fut.poll();
+        }
+
+        match self.fut_a.poll() {


This can just be

let resp = try_ready!(self.fut_a.poll()); self.fut_b = Some(self.b.call(resp)); self.poll()

hawkw · 2018-09-02T00:54:54Z

tower-util/src/ext/and_then.rs

+    type Future = AndThenFuture<A, B>;
+
+    fn poll_ready(&mut self) -> Poll<(), Self::Error> {
+        match self.a.poll_ready() {


This could be:

let _ = try_ready!(self.a.poll_ready()); self.b.poll_ready()

carllerche

Looking good. I left some thoughts inline. Looking forward to the rest.

carllerche · 2018-09-04T17:34:34Z

tower-util/src/ext/and_then.rs

+impl<A, B> AndThen<A, B>
+where
+    A: Service,
+    A::Error: Into<B::Error>,


I would recommend against the Into strategy for matching up error types. This is considered a futures 0.1 mistake (the issue doesn't have much discussion...). The problem is it creates a bunch of type inference issues.

The strategy is to either require both services to have the same error type or make the combinator's error Either<A, B>. In this case, I believe it makes most sense to require thee same error type. The user would be able to map the error w/ map_err.

just figured out inference problems.

carllerche · 2018-09-04T17:39:00Z

tower-util/src/ext/apply.rs

+pub struct Apply<T, F, R, Req> {
+    service: T,
+    f: F,
+    r: PhantomData<Fn(Req) -> R>,


I also think only the request is needed as a generic on the struct?

carllerche · 2018-09-04T17:55:26Z

tower-util/src/ext/map_err.rs

+pub struct MapErr<T, F, E> {
+    service: T,
+    f: F,
+    e: PhantomData<E>,


Is this PhantomData required?

carllerche · 2018-09-04T17:55:58Z

tower-util/src/ext/map_err.rs

+impl<T, F, E> MapErr<T, F, E>
+where
+    T: Service,
+    F: Fn(T::Error) -> E + Clone,


Did Clone on closures land in Rust stable?

yes, it is in 1.26 https://github.com/rust-lang/rust/blob/master/RELEASES.md#version-1260-2018-05-10

carllerche · 2018-09-04T17:58:41Z

tower-util/src/ext/mod.rs

+    fn and_then<B, U>(self, service: U) -> AndThen<Self, B>
+    where
+        Self: Sized,
+        U: Into<B>,


I don't understand why the B service is being converted here.

this would be useful for #69, but i found that From/Into does not work well with type inference.
we need to use custom IntoService trait. i can remove conversion or implement IntoService, it is up to you?

Yeah, I think we should limit Into / From bounds due to type inference issues.

Let's remove the conversion for now.

Separately, I think IntoService will helpful at some point, but lets punt for this PR so that we can get it merged.

carllerche

Thanks 👍 LGTM. I'll probably play around w/ these combinators some after merging.

Is this ready to merge?

fafhrd91 · 2018-09-17T21:52:07Z

Yes, it is ready

carllerche · 2018-09-17T22:04:16Z

Thanks 👍

add combinators for working with Service's

1a4531d

hawkw reviewed Sep 2, 2018

View reviewed changes

carllerche reviewed Sep 4, 2018

View reviewed changes

fafhrd91 added 7 commits September 4, 2018 13:04

do not use Into for error conversion

89c3e87

remove PhantomData from MapErr

2f6055b

add ServiceExt::from_err() combinator

a3ab4c3

match Error type for Apply combinator

fa3c0e7

remove Into conversion

cf2cb78

add Clone implementations

2a20a77

tests for combinators

eb5d6f6

fafhrd91 changed the title ~~[WIP] add combinators for working with Service's~~ Add combinators for working with Service's Sep 11, 2018

hawkw changed the title ~~Add combinators for working with Service's~~ Add combinators for working with Services Sep 11, 2018

carllerche approved these changes Sep 17, 2018

View reviewed changes

carllerche merged commit c29f7e9 into tower-rs:master Sep 17, 2018

Conversation

fafhrd91 commented Sep 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fafhrd91 commented Sep 1, 2018

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carllerche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carllerche left a comment

Choose a reason for hiding this comment

Uh oh!

fafhrd91 commented Sep 17, 2018

Uh oh!

carllerche commented Sep 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fafhrd91 commented Sep 1, 2018 •

edited

Loading