Queue refactor #110
Queue refactor #110
Conversation
| @@ -86,7 +87,7 @@ where | |||
| url.as_str(), | |||
| &http::Method::PUT, | |||
| &|mut request| { | |||
| request = AddAsHeader::add_as_header(self.client_request_id(), request); | |||
| request = add_header(self.client_request_id(), request); | |||
| request = AddAsHeader::add_as_header(&self.metadata(), request); | |||
There was a problem hiding this comment.
metadata is not Option so we cannot use the helper function. Should we create a non option variant? I would be a one liner so I thought better to call the trait function directly.
There was a problem hiding this comment.
It's up to you. In cosmos I wrap the ones that are not options in Some. The compiler most likely can see through this and will skip the if check add_header.
There was a problem hiding this comment.
I think than using wrapping into a fake Option for the sake of saving a function makes for confusing code.
I suggest we have two functions instead: add_optional_header and add_mandatory_header. Their names are self-explanatory even to a casual reader.
| req = crate::headers::add_header(Some(self.is_upsert()), req); | ||
| req = crate::headers::add_header(Some(self.indexing_directive()), req); | ||
| req = crate::headers::add_header(Some(self.allow_tentative_writes()), req); | ||
| req = azure_core::headers::add_optional_header(&self.if_match_condition(), req); |
There was a problem hiding this comment.
I think this is better.
sdk/core/src/metadata.rs
Outdated
| use std::collections::HashMap; | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct Metadata<'a>(HashMap<Cow<'a, str>, Cow<'a, str>>); |
There was a problem hiding this comment.
Same point here about Cow. This makes the type harder to understand and thus harder to use.
There was a problem hiding this comment.
I am on the fence on whether to accept &str or a String. &str means the Metadata struct cannot escape the owning function easily while forcing string means potentially copying hundreds of strings around (unlikely though).
I'll go with String for ergonomics.
There was a problem hiding this comment.
For everything else we're taking &str. I'm fine with going with String. We might want to think about why Metadata is special and takes String. Perhaps more types should take String instead of &str. Something to think about....
There was a problem hiding this comment.
My reasoning is: for something that does not make sense outside a request (like client_request_id) &str is fine. This is because you use it on a request only, and a requests are short lived 99% of the time (so borrowing makes sense).
For everything else String is better.
Building on #109, this is a draft of the same changes on azure queue storage.
This PR follows the same patterns and proposes some new types (
Timeout,ClienrtRequestIdandMetadata), removes the trait functions (now simply struct functions) for the builder(s).I am not happy with the
Metadatastruct so suggestions are welcome: for the time being it's a string owning hashmap but maybe a &str based one would be better? A Cow to give the user liberty to choose?