-
Notifications
You must be signed in to change notification settings - Fork 327
http-types and async-h1 2.0.0 #520
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
Conversation
I've updated
|
@jbr Waiting for a new version of |
@yoshuawuyts Well it's reassuring I'm not the only one surprised by that 😄, and glad to see it's in the works |
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.
Overall looking really good! -- think we'll have to follow this up with a PR to update our body methods, but that can be done after we merge this
pub fn set_header(mut self, key: impl Into<HeaderName>, value: impl ToHeaderValues) -> Self { | ||
self.res.insert_header(key, value); | ||
self |
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.
Oh actually, we should probably remove the Self
return type from all of these methods. They should strictly forward to http-types
here.
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.
these tests are so much nicer now :)
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.
Looks good!
A few notes on this mostly-find-and-replace PR:
This is building against async-h1 update for http-types 2.0.0 async-h1#105 and http-types master, as there are not yet releases for those
This PR revealed an issue with surf's native client when talking to a server that understands 100-continue. instead of attempting to resolve that, this pr switches to surf's async-h1 client for testing. Problem with
Expect: 100-continue
handling with isahc surf#177 represents further work required to address thisInteracting with a single header value is still a little awkward and this PR is inconsistent about using [0] vsHeaderValues::last()
It's surprising that
Response::append_header
panics if the ToHeaderValues cannot be converted butResponse::set_header
does notTests that rely on surf are still a little awkward because of incompatible http-types
An unexpected side effect of accepting TryIntourl::Url for http_types::Request is that they can no longer be constructed with an implicit
url::Url
as in"str".parse().unwrap()
, but instead need to be explicitly parsed asurl::Url::parse("str").unwrap()
. I was surprised to discover that Url isn'tTryFrom<&str>
.