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

Rename ServiceExt::ready_and to ServiceExt::ready #567

Merged
merged 4 commits into from
Feb 25, 2021

Conversation

davidbarsky
Copy link
Contributor

This PR renames:

  • ServiceExt::ready_and to ServiceExt::ready
  • the ReadyAnd future to Ready
  • the associated documentation to refer to ServiceExt::ready and ReadyAnd.

My recollection of the original conversation surrounding the introduction of the ServiceExt::ready_and combinator in #427 was that it was meant to be a temporary workaround for the unchainable ServiceExt::ready combinator until the next breaking release of the Tower crate. The unchainable ServiceExt::ready combinator was removed, but ServiceExt::ready_and was not renamed. I believe, but am not 100% sure, that this was an oversight.

I also recognize that this PR might have a large impact. To reduce the impact, I'd be happy to modify this PR to introduce ServiceExt::ready and deprecate ServiceExt::ready_and.

@davidbarsky davidbarsky requested review from hawkw, davidpdrsn, jonhoo, olix0r and a team February 24, 2021 20:40
@hawkw
Copy link
Member

hawkw commented Feb 24, 2021

I also recognize that this PR might have a large impact. To reduce the impact, I'd be happy to modify this PR to introduce ServiceExt::ready and deprecate ServiceExt::ready_and.

let's go ahead and do this, please — that way, we can land this now, and remove ready_and in 0.5.

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

Yes! I've also thought that ready_and was an awkward name.

@davidbarsky
Copy link
Contributor Author

let's go ahead and do this, please — that way, we can land this now, and remove ready_and in 0.5.

Done! I've also opened #568 as a reminder.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

couple of nits --- can we fix the new warnings added in this PR? i think the other ones were already present, so we can fix those in a follow-up, but this adds new deprecation warnings due to internal use of deprecated APIs.

besides that, lgtm!

tower/src/util/mod.rs Show resolved Hide resolved
tower/src/util/mod.rs Show resolved Hide resolved
tower/src/util/mod.rs Outdated Show resolved Hide resolved
tower/src/util/ready.rs Show resolved Hide resolved
Copy link
Member

@hawkw hawkw 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 — thanks for fixing the internal deprecation warnings!

@hawkw hawkw merged commit a8884ae into master Feb 25, 2021
@hawkw hawkw deleted the davidbarsky/rename-ready-and-to-ready branch February 25, 2021 17:37
hawkw added a commit that referenced this pull request Feb 27, 2021
Deprecated

- **util**: Deprecated `ServiceExt::ready_and` (renamed to
  `ServiceExt::ready`). ([#567])
- **util**: Deprecated `ReadyAnd` future (renamed to `Ready`). ([#567])

Added

- **builder**: Add `ServiceBuilder::layer_fn` to add a layer built from
  a function. ([#560])
- **builder**: Add `ServiceBuilder::map_future` for transforming the
  futures produced by a service. ([#559])
- **builder**: Add `ServiceBuilder::service_fn` for applying `Layer`s to
  an async function using `util::service_fn`. ([#564])
- **util**: Add example for `service_fn`. ([#563])
- **util**: Add `BoxLayer` for creating boxed `Layer` trait objects.
  ([#569])

[#567]: #567
[#560]: #560
[#559]: #559
[#564]: #564
[#563]: #563
[#569]: #569
davidpdrsn pushed a commit that referenced this pull request Feb 27, 2021
Deprecated

- **util**: Deprecated `ServiceExt::ready_and` (renamed to
  `ServiceExt::ready`). ([#567])
- **util**: Deprecated `ReadyAnd` future (renamed to `Ready`). ([#567])

Added

- **builder**: Add `ServiceBuilder::layer_fn` to add a layer built from
  a function. ([#560])
- **builder**: Add `ServiceBuilder::map_future` for transforming the
  futures produced by a service. ([#559])
- **builder**: Add `ServiceBuilder::service_fn` for applying `Layer`s to
  an async function using `util::service_fn`. ([#564])
- **util**: Add example for `service_fn`. ([#563])
- **util**: Add `BoxLayer` for creating boxed `Layer` trait objects.
  ([#569])

[#567]: #567
[#560]: #560
[#559]: #559
[#564]: #564
[#563]: #563
[#569]: #569
hawkw added a commit that referenced this pull request Mar 1, 2021
Deprecated

- **util**: Deprecated `ServiceExt::ready_and` (renamed to
  `ServiceExt::ready`). ([#567])
- **util**: Deprecated `ReadyAnd` future (renamed to `Ready`). ([#567])

Added

- **builder**: Add `ServiceBuilder::layer_fn` to add a layer built from
  a function. ([#560])
- **builder**: Add `ServiceBuilder::map_future` for transforming the
  futures produced by a service. ([#559])
- **builder**: Add `ServiceBuilder::service_fn` for applying `Layer`s to
  an async function using `util::service_fn`. ([#564])
- **util**: Add example for `service_fn`. ([#563])
- **util**: Add `BoxLayer` for creating boxed `Layer` trait objects.
  ([#569])

[#567]: #567
[#560]: #560
[#559]: #559
[#564]: #564
[#563]: #563
[#569]: #569

Signed-off-by: Eliza Weisman <[email protected]>
hawkw added a commit that referenced this pull request Mar 11, 2022
These was deprecated in #567. In 0.5, we can just remove them entirely.
hawkw added a commit that referenced this pull request Mar 11, 2022
These was deprecated in #567. In 0.5, we can just remove them entirely.
hawkw added a commit that referenced this pull request Mar 11, 2022
These were deprecated in #567. In 0.5, we can just remove them
entirely.

Closes #568

Signed-off-by: Eliza Weisman <[email protected]>
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.

4 participants