-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Chore: Move OnUpgrade state from Body to Request/Response #2353
Conversation
@seanmonstar I came up with a scheme that I think makes sense, storing OnUpgrade state in MessageHead::extensions and moving the state into Response::extensions and Request::extensions. I get a compilation error due to missing trait implementations for Extensions however, could you help me understand how to solve them please? Also, just point out if I should simply have solved it differently :)
|
Those are probably from |
@seanmonstar Thanks, I thought those traits were necessary. Removed them, fixed the compile :) |
I'll rebase to fix commit messages, since everything else looks fine. |
63c883a
to
6a22a2f
Compare
ca8d55b
to
60238f9
Compare
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.
Fantastic work! This is exactly what I had in mind. Just one comment inline.
Move state required for protocol upgrades to head representations, instead of associating it with the body. Closes hyperium#2340. Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Thanks @seanmonstar! I applied your suggestion. BTW, I amended into the original commit, and rebased. Do you prefer this way of working on PRs, or would you prefer if I avoid rewriting the commit history (i.e., just merge, add new commits)? |
Move state required for protocol upgrades to head representations, instead of associating it with the body. Closes hyperium#2340. Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Move OnUpgrade state from Body to http::Request/http::Response extensions.
Fixes #2340.