various fixes needed to integrate into surf + tide #44
Conversation
dignifiedquire
changed the title
dignifiedquire
changed the title
| @@ -1,18 +1,19 @@ | |||
| //! Process HTTP connections on the client. | |||
|
|
|||
| use async_std::io::{self, BufReader}; | |||
| use async_std::io::{self, BufReader, Read}; | |||
There was a problem hiding this comment.
It would be nice actually if we could remove our dependency on async-std and only rely on futures-rs. I don't think this is currently possible because we rely on a timer, but it would be nice to not move away from that direction.
There was a problem hiding this comment.
@rylev we're going to need to use channels either way, so we'll have to depend on async-std. And you're right that since we depend on timers we have a dependency either way. I guess the one upside to this though is that it'll work with any runtime out of the box, even if there might be multiple executors running.
|
|
||
| #[async_std::test] | ||
| async fn test_basic_request() { | ||
| let case = TestCase::new("fixtures/request1.txt", "fixtures/response1.txt").await; |
There was a problem hiding this comment.
Maybe we should rename these files to: basic-request.txt and basic-response.txt
There was a problem hiding this comment.
Overall this is looking really good; a few minor nits but nothing critical. Especially the addition of thorough tests inspires confidence.
Thanks so much @dignifiedquire; this is excellent!
| ); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
All put together this file feels pretty long. Is there perhaps a chance we could split it up into a few smaller parts?
Also as @rylev pointed out there are several instances of mem::replace that could probably be replaced by reassigning instead?
With the list of these fun repos I made surf + tide work with async-h1