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

Implement try_chunks #2438

Merged
merged 2 commits into from
May 17, 2021
Merged

Implement try_chunks #2438

merged 2 commits into from
May 17, 2021

Conversation

ivan770
Copy link
Contributor

@ivan770 ivan770 commented May 11, 2021

Closes #1947

@ivan770 ivan770 requested a review from taiki-e as a code owner May 11, 2021 12:46
taiki-e
taiki-e reviewed May 15, 2021
View reviewed changes
Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I'm ok with adding such a feature, but I'm not sure if the behavior when an error occurs is good with this.

I feel it might be preferable to have our own error type that contains the error returned by the stream and the elements of the buffer at the time the error occurred.
What do you think?

@taiki-e taiki-e added A-stream Area: futures::stream S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author labels May 15, 2021
@ivan770
Copy link
Contributor Author

ivan770 commented May 15, 2021

Thanks for the PR! I'm ok with adding such a feature, but I'm not sure if the behavior when an error occurs is good with this.

I feel it might be preferable to have our own error type that contains the error returned by the stream and the elements of the buffer at the time the error occurred.
What do you think?

Yeah, I thought about adding a tuple of all elements before error and the error itself, but my approach would lead to conversion of TryStream to Stream. I think your approach with custom error type seems more suitable, as TryStream is preserved. I'll modify PR a bit later.

@taiki-e taiki-e removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author label May 17, 2021
@ivan770
Copy link
Contributor Author

ivan770 commented May 17, 2021

Sorry for lot's of intermediate commits and CI activation requests. I guess simple ID verification could be better for GitHub Actions 😅

taiki-e
taiki-e approved these changes May 17, 2021
View reviewed changes
Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks!

@taiki-e taiki-e merged commit 5f610f8 into rust-lang:master May 17, 2021
@taiki-e taiki-e added the 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. label May 17, 2021
Ten0 added a commit to Ten0/futures-rs that referenced this pull request Jul 23, 2021
@Ten0
Backport try_chunks to 0.3
415ed37 
rust-lang#2438 is marked as `0.3-backport: pending`.
This is it. :)

Resolves rust-lang#1947 (which specifically mentionned 0.3)
@Ten0 Ten0 mentioned this pull request Jul 23, 2021
Closed
taiki-e pushed a commit that referenced this pull request Jul 23, 2021
@ivan770 @taiki-e
Implement try_chunks (#2438)
2e6f688
@taiki-e taiki-e mentioned this pull request Jul 23, 2021
Merged
@taiki-e taiki-e added 0.3-backport: completed and removed 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. labels Jul 23, 2021
taiki-e pushed a commit that referenced this pull request Jul 23, 2021
@ivan770 @taiki-e
Implement try_chunks (#2438)
b32fbc4
@taiki-e taiki-e mentioned this pull request Jul 23, 2021
Merged
@taiki-e
Copy link
Member

taiki-e commented Jul 23, 2021

Published in 0.3.16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taiki-e taiki-e taiki-e approved these changes

Assignees
No one assigned
Labels
0.3-backport: completed A-stream Area: futures::stream
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

try_chunks for futures 0.3
2 participants
@ivan770 @taiki-e