Move to tokio 0.2 and futures 3 and make groups api async #81
Conversation
coltfred
changed the title
src/internal/rest.rs
Outdated
| use percent_encoding::SIMPLE_ENCODE_SET; | ||
| use reqwest::{ | ||
| header::HeaderMap, | ||
| r#async::{Client as RClient, Request as ARequest}, |
There was a problem hiding this comment.
| r#async::{Client as RClient, Request as ARequest}, | |
| r#async::{Client as RClient, Request}, |
Can this just be Request now, or something more specific like Reqwest if the intent is to differentiate it from IronCoreRequest?
There was a problem hiding this comment.
I agree that we should just leave it as Request. ARequest actually came in when we were also using the blocking (then default) Request as well.
src/internal/rest.rs
Outdated
| use percent_encoding::SIMPLE_ENCODE_SET; | ||
| use reqwest::{ | ||
| header::HeaderMap, | ||
| r#async::{Client as RClient, Request as ARequest}, |
There was a problem hiding this comment.
Pretty sure the async module is deprecated... maybe? Also there are two reqwest use blocks here. They should probably be combined.
| let parsed_url = Url::parse(encoded_full_url)?; | ||
| let query_str_format = |q: &str| format!("?{}", q); | ||
| let query_str_format = |q: &str| format!("?{}", q.replace("%27", "'")); |
There was a problem hiding this comment.
There should probably be a comment here explaining what's going on here.
| .map(|sig_url| auth_b.finish_with(sig_url, req.method().clone(), Some(&body_bytes))) | ||
| .map_err(|e| IronOxideErr::from((e, error_code))); | ||
|
|
||
| // we only support Authorization::Version2 with this call |
There was a problem hiding this comment.
There was some hope that the duplicated code between here and post_raw might be able to be easily refactored once we got rid of v1 futures. Is that worth taking a pass at, do you think?
src/internal/rest.rs
Outdated
| @@ -1338,7 +1332,7 @@ mod tests { | |||
| maybe_path.unwrap().signature_string(), | |||
| "/api/1/users?id=user-10" | |||
| ); | |||
|
|
|||
| // | |||
|
You should also write the change log entry for moving to futures 3, async/await, and tokio 0.2 |
src/internal/rest.rs
Outdated
| header::HeaderMap, | ||
| r#async::{Client as RClient, Request as ARequest}, | ||
| header::{HeaderMap, HeaderValue, CONTENT_TYPE}, | ||
| r#async::{Client as RClient, Request as ARequest, RequestBuilder}, |
There was a problem hiding this comment.
did you figure out that the async namespace is still the preferred way to import here?
https:/seanmonstar/reqwest/blob/80ba8cc15066c2a6d35d68b058e795be5e01cd94/src/lib.rs#L301 makes me think otherwise.
There was a problem hiding this comment.
No, I just pushed the fix.
| @@ -186,6 +184,8 @@ pub struct SignatureUrlString(String); | |||
| impl SignatureUrlString { | |||
| pub fn new(encoded_full_url: &str) -> Result<SignatureUrlString, url::ParseError> { | |||
| let parsed_url = Url::parse(encoded_full_url)?; | |||
| //The formatter for the query string is overfomatting compared to what we want, so we want to replace | |||
There was a problem hiding this comment.
Do you think this is worth opening an issue on reqwest? Something like "Support configurable query string encoding". Perhaps if we reference our issue someone will point out a better way all together?
TODO