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

Add trait StreamOperation with stop_operation, wrapper SubscriptionStream. #19

Closed
wants to merge 4 commits into from
Closed

Add trait StreamOperation with stop_operation, wrapper SubscriptionStream. #19

wants to merge 4 commits into from

Conversation

BratSinot
Copy link
Contributor

Implements #18
Based on errors #17

@obmarg
Copy link
Owner

obmarg commented May 26, 2021

Hey @BratSinot - this looks good at first glance. Do you mind merging/rebasing in master please and then I'll take another look. Thanks

@BratSinot
Copy link
Contributor Author

Hey @BratSinot - this looks good at first glance. Do you mind merging/rebasing in master please and then I'll take another look. Thanks

Done.

@BratSinot
Copy link
Contributor Author

BratSinot commented May 27, 2021 •
edited
Loading

I'm not sure about returning impl StreamOperation + Stream<Item = Operation::Response>. I'm still learning Rust and didn't find the way to combine traits together.
And returning struct will be pain because of receiver.map(move |response| op.decode(response).unwrap()). For same reason I return trait StreamOperation + Stream.

@obmarg
Copy link
Owner

obmarg commented May 27, 2021

I'm not sure about returning impl StreamOperation + Stream<Item = Operation::Response>. I'm still learning Rust and didn't find the way to combine traits together.
And returning struct will be pain because of receiver.map(move |response| op.decode(response).unwrap()). For same reason I return trait StreamOperation + Stream.

Yeah, this is a good point. I may take a closer look at this before I merge.

Sorry it's taking me so long to get back to you on all these reviews - been a busy week at work and haven't had much time for side projects. Thanks for your patience (and of course the contributions) 🙏

@BratSinot BratSinot mentioned this pull request May 28, 2021
Merged
@obmarg
Copy link
Owner

obmarg commented May 29, 2021

I've taken your code and tried to simplify it a bit @BratSinot - it now just returns a concrete type rather than an impl StreamOperation + Stream. Thought it was simpler that way.

Closing this in favour of #23 (which is based off this branch, just couldn't figure out how to push to this PR easily).

I still want to do a bit of work to get an impl Drop on the SubscriptionStream, but it'll need more work than I have the time for just now.

@obmarg obmarg closed this May 29, 2021
@obmarg obmarg mentioned this pull request May 30, 2021
Open
@BratSinot BratSinot deleted the stream_operations branch May 30, 2021 14:13
obmarg pushed a commit that referenced this pull request May 30, 2021
@BratSinot
Unwrap all unwrap() and expect() to actual errors. (#20)
8ffb424 
Fixes #4 
Also playing a little after merge this with into #19 (in my branch `all_changes`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@BratSinot @obmarg