Conversation
`ServiceExt::ready` says that it produces "A future yielding the service when it is ready to accept a request." This is not true; it does _not_ yield the service when it is ready, it yields unit. This makes it impossible to chain service ready with service call, which is sad. This PR adds `ready_and`, which does what `ready` promised. It also deprecates `ready` with the intention that we remove `ready` in a future version, and make the strictly more general `ready_and` take its place. We can't do it now since it's not a backwards-compatible change even though it _probably_ wouldn't break any code. The PR also updates the docs so that they reflect the observed behavior.
LucioFranco
reviewed
Mar 23, 2020
| where | ||
| T: Service<Request>, | ||
| { | ||
| #[allow(missing_docs)] |
Member
There was a problem hiding this comment.
should we just add docs now :)
Collaborator
Author
There was a problem hiding this comment.
I always found documentation on new super weird since it becomes so trivial (for simple cases like this). I guess I could add something, but my inclination is to leave it 😅
LucioFranco
reviewed
Mar 23, 2020
| pub struct Ready<'a, T, Request> { | ||
| inner: &'a mut T, | ||
| /// `ReadyOneshot` values are produced by `ServiceExt::ready_oneshot`. | ||
| pub struct ReadyOneshot<T, Request> { |
Member
There was a problem hiding this comment.
Do we want a manual clone impl if T is clone?
Collaborator
Author
There was a problem hiding this comment.
My first instinct says no — it should be rare that you'd want to clone a ReadyOneshot as opposed to cloning the underlying Service. If you did, that's probably not what you meant to do.
LucioFranco
reviewed
Mar 23, 2020
LucioFranco
approved these changes
Mar 23, 2020
Member
LucioFranco
left a comment
LucioFranco
left a comment
There was a problem hiding this comment.
Left a few comments, nothing blocking :)
hawkw
pushed a commit
that referenced
this pull request
Feb 25, 2021
This PR renames: - `ServiceExt::ready_and` to `ServiceExt::ready` - the `ReadyAnd` future to `Ready` - the associated documentation to refer to `ServiceExt::ready` and `ReadyAnd`. This PR deprecates: - the `ServiceExt::ready_and` method - the `ReadyAnd` future These can be removed in Tower 0.5. 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ServiceExt::readysays that it produces "A future yielding the servicewhen it is ready to accept a request." This is not true; it does not
yield the service when it is ready, it yields unit. This makes it
impossible to chain service ready with service call, which is sad.
This PR adds
ready_and, which does whatreadypromised. It alsodeprecates
readywith the intention that we removereadyin a futureversion, and make the strictly more general
ready_andtake its place.We can't do it now since it's not a backwards-compatible change even
though it probably wouldn't break any code.
The PR also updates the docs so that they reflect the observed behavior.