-
Notifications
You must be signed in to change notification settings - Fork 168
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 http-body 1.0 #348
Conversation
sleep_data: Option<Sleep>, | ||
#[pin] | ||
sleep_trailers: Option<Sleep>, | ||
sleep: Option<Sleep>, |
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.
@LucioFranco does the changes here seem right to you?
2ff894c
to
9fad529
Compare
Hey. New to tower-http codebase. Complex tasks would certainly take me time to understand and work on. Are there any low-hanging fruits that I can help with, and then I can slowly work my way up? Just want to see if there's any way I can help push this ahead |
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.
Some more small comments, have now reviewed completely and looks good to me overall (obviously 😄).
@@ -1,3 +1,8 @@ | |||
fn main() { | |||
eprintln!("this example has not yet been updated to hyper 1.0"); |
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 not? I did it with little effort for the tower-async
version. Want me to contribute this as a PR to your PR?
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.
Same reason as #348 (comment), axum doesn't yet support http 1.0
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.
True. For now I was using your branch. I get you might not want to do that, but now you do not have an example at all... so dunno.
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.
axum doesn't yet support http 1.0
but now it does, right?
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.
Yep it does.
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.
A PR to restore this example using axum 0.7: #448
@@ -1,3 +1,8 @@ | |||
fn main() { | |||
eprint!("this example has not yet been updated to hyper 1.0"); |
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 not? I did it with little effort for the tower-async
version. Want me to contribute this as a PR to your PR?
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.
How did you port it? Tonic doesn't support http 1.0. Did you do a bunch of mapping of the types?
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.
Sorry I was wrong. Tonic and Warp examples I deleted during my original port as I do not use these libraries. I only kept in the hyper and axum ones and those are the only once I fixed this time as well.
@@ -39,6 +39,10 @@ impl AllowPrivateNetwork { | |||
Self(AllowPrivateNetworkInner::Predicate(Arc::new(f))) | |||
} | |||
|
|||
#[allow( |
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.
Hmmm, I wonder why I don't need these on tower-async. Are we running different versions of clippy or different configs?
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.
Not sure. I really wanna cut down on the number of feature flags we use anyway. A job for another day 😅
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.
Besides the few, mostly open or nitpick comment. All looks excellent to me.
Hm, we do not have an example on how to combine this w/ |
Still some things to do but should allow tokio-rs/axum#1882 to move forward.
TODO
Frame::map_data
Limited
. Should we create wrappers?