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.
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
Upgrade to Tokio 1.0 #753
Upgrade to Tokio 1.0 #753
Changes from 28 commits
e3c4c70
fc71e96
610f3dc
d4c9344
6308e90
2de7e08
305c888
b476d81
fa8a9e8
5d068ae
f242193
3f038da
2dd9bcf
7bfc760
9722b25
7f97567
127fff2
79df1c5
2df48ca
f6508fa
3c1d0ad
2f2206c
a9f2885
7ff37e3
f98e363
6326d05
b9ef973
33ede73
e50c375
d8bfb02
e989869
4d92fa8
657f1c6
bb2f511
5ac7a62
b095a6d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you prefer
FramedRead
overReaderStream
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jxs To be honest, I don't remember exactly, but it was probably from an example. Do you think ReaderStream is preferable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was inspired from the reqwest implementation https://github.com/seanmonstar/reqwest/blob/a19eb3419637856d6d42f55dff6e4790613b2933/src/async_impl/decoder.rs#L47
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @paolobarbolini, I can't bring myself to remember where I found this solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @paolobarbolini. @aknuds1 I asked because I was not sure either, Previously
FrameReader
was the way to achieveAsyncRead
toStream
conversion, but sinceReaderStream
was introduced I would maybe suggestReaderStream
as it's simpler, and recommended for this cases on tokio-util docThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanmonstar Do you have any opinion on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I converted from FramedRead into ReaderStream, ass @jxs recommended. I don't really know personally if there are any practical differences, but I can tell that the API is simpler (a second argument to
FramedRead::new
can be dropped). I trust @jxs knows more about this than I do, and I'll let @seanmonstar be the judge.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you already have the extension, you don't need to insert it here to remove it again. (
on_upgrade
is the same asfut
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanmonstar I think I see what you mean, and try to simplify it to the best of my ability. Please have a look.