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

Change Discover to be a sealed trait #443

Merged
merged 4 commits into from
Apr 17, 2020
Merged

Change Discover to be a sealed trait #443

merged 4 commits into from
Apr 17, 2020

Conversation

jonhoo
Copy link
Collaborator

@jonhoo jonhoo commented Apr 17, 2020

Discover was really just a TryStream<Item = Change>, so this
change makes that much clearer. Specifically, users are intended to use
Discover only in bounds, whereas implementors should implement
Stream with the appropriate Item type. Discover then comes with a
blanket implementation for anything that implements TryStream
appropriately. This obviates the need for the discover::stream module.

@jonhoo
Change Discover to be a sealed trait
ba3302c 
`Discover` was _really_ just a `TryStream<Item = Change>`, so this
change makes that much clearer. Specifically, users are intended to use
`Discover` only in bounds, whereas implementors should implement
`Stream` with the appropriate `Item` type. `Discover` then comes with a
blanket implementation for anything that implements `TryStream`
appropriately. This obviates the need for the `discover::stream` module.
@jonhoo jonhoo added A-discover Area: The tower "discover" middleware C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Apr 17, 2020
@jonhoo
Copy link
Collaborator Author

jonhoo commented Apr 17, 2020

I'd like to get @olix0r's take on this too, since he's the one (as far as I know) who's been most involved with Discover.

@jonhoo jonhoo added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 17, 2020
@jonhoo jonhoo mentioned this pull request Apr 17, 2020
Closed
33 tasks
@olix0r
Copy link
Collaborator

olix0r commented Apr 17, 2020 •
edited
Loading

The intent for Discover, as differentiated from Stream, which exposes optionality, was to allow consumers to assume the stream would never complete.

All that said, I don't think it really matters. It can be up to the consumer what to do when the stream ends...

@jonhoo
Copy link
Collaborator Author

jonhoo commented Apr 17, 2020

Yeah, I think it makes more sense for the consumer to be able to distinguish between Poll::Ready(None) and Poll::Pending. They may decide to treat the two the same, or they may decide that one can be handled more efficiently by not polling again in the future.

@jonhoo jonhoo merged commit c87fdd9 into master Apr 17, 2020
@jonhoo jonhoo deleted the jonhoo/spring-clean branch April 17, 2020 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LucioFranco LucioFranco LucioFranco approved these changes

@olix0r olix0r Awaiting requested review from olix0r

Assignees
No one assigned
Labels
A-discover Area: The tower "discover" middleware C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jonhoo @olix0r @LucioFranco