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.

Sign up for GitHub

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

Jump to bottom

combinator service unification #498

Closed
hawkw opened this issue Dec 28, 2020 · 4 comments · Fixed by #499
Closed

combinator service unification #498

hawkw opened this issue Dec 28, 2020 · 4 comments · Fixed by #499
Assignees
@hawkw
Labels
A-builder Area: Tower's service builder A-util Area: The tower "util" module C-cleanup Category: PRs that clean code up or issues documenting cleanup. P-high High priority relnotes Marks issues that should be documented in the release notes of the next release.
Milestone
v0.4

Comments

@hawkw
Copy link
Member

hawkw commented Dec 28, 2020

Currently, the ServiceExt trait has with and try_with methods for composing a Service with functions that modify the request type, and map_err and map_ok methods for composing a service with functions that modify the response type. Meanwhile, ServiceBuilder has map_request for composing a service with a request mapping function, and map_response for composing a service with a response-mapping function. These combinators are very similar in purpose, but are implemented using different middleware types that essentially duplicate the same behavior.

Before releasing 0.4, we should probably unify these APIs. In particular, it would be good to de-duplicate the middleware service types, and to unify the naming.

Personally, I think the ServiceExt versions of map_X have a slightly better design, since they take FnOnce + Clone, rather than taking a Fn and Arcing it internally. This avoids the overhead of Arc for mapping functions that are trivially clonable (e.g. function pointers) but still allows the user to use Arcs when needed.

I'm open to bikeshedding what naming to settle on. with has an analogy with the futures crate's Sink methods, but I personally find map_request and map_response a bit clearer in the tower context. However, if we want to have both Err and Ok versions of these combinators, which I think we do, map_response_err and map_response_ok gets a bit wordy...perhaps we could just have map_response and map_err?
@hawkw hawkw self-assigned this Dec 28, 2020
@hawkw
Copy link
Member Author

hawkw commented Dec 28, 2020

cc @LucioFranco @hlb8122

@hawkw hawkw added A-builder Area: Tower's service builder A-util Area: The tower "util" module C-cleanup Category: PRs that clean code up or issues documenting cleanup. P-high High priority relnotes Marks issues that should be documented in the release notes of the next release. labels Dec 28, 2020
@seanmonstar
Copy link
Collaborator

map_response_err and map_response_ok

Drive-by: I prefer map_request, map_response, and map_err. 🏎️

hawkw added a commit that referenced this issue Dec 28, 2020
@hawkw
util: unify and rename combinators
14a3229 
Currently, the `ServiceExt` trait has `with` and `try_with` methods for
composing a `Service` with functions that modify the request type, and
`map_err` and `map_ok` methods for composing a service with functions
that modify the response type. Meanwhile, `ServiceBuilder` has
`map_request` for composing a service with a request mapping function,
and `map_response` for composing a service with a response-mapping
function. These combinators are very similar in purpose, but are
implemented using different middleware types that essentially duplicate
the same behavior.

Before releasing 0.4, we should probably unify these APIs. In
particular, it would be good to de-duplicate the middleware service
types, and to unify the naming.

This commit makes the following changes:
- Rename the `ServiceExt::with` and `ServiceExt::try_with` combinators
  to `map_request` and `try_map_request`
  - Rename the `ServiceExt::map_ok` combinator to `map_response`
- Unify the `ServiceBuilder::map_request` and `ServiceExt::map_request`
  combinators to use the same `Service` type
- Unify the `ServiceBuilder::map_response` and
  `ServiceExt::map_response` combinators to use the same `Service` type
- Unify the `ServiceBuilder::map_err` and `ServiceExt::map_err`
  combinators to use the same `Service` type
- Only take `FnOnce + Clone` when in response/err combinators, which
  require cloning into the future type. `MapRequest` and `TryMapRequest`
  now take `FnMut(Request)`s and don't clone them every time they're
  called
- Reexport future types for combinators where it makes sense.
- Add a `try_map_request` method to `ServiceBuilder`

Closes #498
@hawkw hawkw mentioned this issue Dec 28, 2020
Merged
@hawkw hawkw added this to the v0.4 milestone Dec 29, 2020
@LucioFranco
Copy link
Member

Drive-by: I agree with sean

@hlb8122
Copy link
Contributor

hlb8122 commented Jan 4, 2021 •
edited
Loading

map_request and map_err make sense to me and are inline with map_ok and map_err from TryFutureExt.

@hawkw hawkw closed this as completed in #499 Jan 4, 2021
hawkw added a commit that referenced this issue Jan 4, 2021
@hawkw
util: unify and rename combinators (#499)
878b10f 
Currently, the `ServiceExt` trait has `with` and `try_with` methods for
composing a `Service` with functions that modify the request type, and
`map_err` and `map_ok` methods for composing a service with functions
that modify the response type. Meanwhile, `ServiceBuilder` has
`map_request` for composing a service with a request mapping function,
and `map_response` for composing a service with a response-mapping
function. These combinators are very similar in purpose, but are
implemented using different middleware types that essentially duplicate
the same behavior.

Before releasing 0.4, we should probably unify these APIs. In
particular, it would be good to de-duplicate the middleware service
types, and to unify the naming.

This commit makes the following changes:
- Rename the `ServiceExt::with` and `ServiceExt::try_with` combinators
  to `map_request` and `try_map_request`
  - Rename the `ServiceExt::map_ok` combinator to `map_response`
- Unify the `ServiceBuilder::map_request` and `ServiceExt::map_request`
  combinators to use the same `Service` type
- Unify the `ServiceBuilder::map_response` and
  `ServiceExt::map_response` combinators to use the same `Service` type
- Unify the `ServiceBuilder::map_err` and `ServiceExt::map_err`
  combinators to use the same `Service` type
- Only take `FnOnce + Clone` when in response/err combinators, which
  require cloning into the future type. `MapRequest` and `TryMapRequest`
  now take `FnMut(Request)`s and don't clone them every time they're
  called
- Reexport future types for combinators where it makes sense.
- Add a `try_map_request` method to `ServiceBuilder`

Closes #498

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hawkw hawkw

Labels
A-builder Area: Tower's service builder A-util Area: The tower "util" module C-cleanup Category: PRs that clean code up or issues documenting cleanup. P-high High priority relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Milestone
v0.4
Development

Successfully merging a pull request may close this issue.

util: unify and rename combinators tower-rs/tower
4 participants
@seanmonstar @hawkw @LucioFranco @hlb8122