use hyperium/http instead of http-rs/http-types #128
Conversation
flisky
force-pushed
the
hyperium_http
branch
from
40c44d3 to
98a69d9
Compare
|
friendly ping |
Friendly tip: never, ever, do this on an OSS project tracker 😄 I review PRs when I have the capacity to, pinging me doesn't make me review it any sooner. |
While I agree that the above ping 2d after the last update was not very helpful, as a maintainer of many projects I want to note that forgetting about a PR is a thing I've been glad that people pinged me after a few weeks of inactivity on multiple occasions. |
src/matchers.rs
Outdated
| } | ||
| values.into_iter().all(|value| self.1.contains(&value)) |
There was a problem hiding this comment.
This has a different semantic compared to the previous one: this is order-insensitive, while the previous implementation was order-sensitive.
A few weeks is fine, but 2 days is not—I don't even ping co-workers that soon 😅 |
src/request.rs
Outdated
| pub(crate) async fn from_hyper(request: hyper::Request<hyper::Body>) -> Request { | ||
| let (parts, body) = request.into_parts(); | ||
| let method = parts.method.into(); | ||
| let (method, headers) = (parts.method, parts.headers); |
There was a problem hiding this comment.
Just my 2c, I'd do this as two statements.
src/response_template.rs
Outdated
| pub(crate) fn generate_response(&self) -> Response { | ||
| let mut response = Response::new(self.status_code); | ||
| pub(crate) fn generate_response(&self) -> Response<Body> { | ||
| let mut response = Response::default(); |
There was a problem hiding this comment.
The Response::builder API should make this code a bit shorter.
src/respond.rs
Outdated
| @@ -75,7 +75,7 @@ use crate::{Request, ResponseTemplate}; | |||
| /// `Respond` that propagates back a request header in the response: | |||
| /// | |||
| /// ```rust | |||
| /// use http_types::headers::HeaderName; | |||
| /// use hyper::header::HeaderName; | |||
There was a problem hiding this comment.
Can't we import from http here? I don't think we need to import from hyper.
There was a problem hiding this comment.
I think it would require an extra (dev) dependency. (not to say that's not worth it)
There was a problem hiding this comment.
It would, but http is already in our (non-dev) dependency tree, so it doesn't make an actual difference.
There was a problem hiding this comment.
Yeah, just one more version to keep in sync with the test in the manifest.
|
This looks good to go, just some conflicts to resolve—thanks for the work! |
|
Both |
|
Glad to hear! Will give it a try on this weekends. |
|
So here is the hyper v1 upgrade. However, I have to use a multi-threaded runtime to run the tests (see my last commit). |
|
I discovered a while ago that converting a |
|
Yes, it's exactly the problem! I push another commit to switch back to single-thread runtime, thanks! |
flisky
force-pushed
the
hyperium_http
branch
from
08da443 to
f98f745
Compare
Fix #123
Please attention: the exported types
Method/HeaderValuesmake this a breaking change.By the way,
hyperv1.0 will removehyper::Body, but I think it's fine to keep it for now?By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.