-
Notifications
You must be signed in to change notification settings - Fork 252
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
Move create_collection over to new architecture #279
Conversation
@@ -1,5 +1,6 @@ | |||
pub use crate::etag::Etag; | |||
pub use crate::request_options::*; | |||
pub use crate::{ | |||
new_http_client, AddAsHeader, AppendToUrlQuery, HttpClient, RequestId, SessionToken, EMPTY_BODY, |
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.
If we expect this to change often, probably best to put them on new lines to make diffs easier to review.
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.
I actually hope this won't change often or at least not after some initial churn.
sdk/cosmos/examples/document_00.rs
Outdated
DATABASE, | ||
azure_cosmos::operations::create_database::Options::new(), | ||
) | ||
.create_database(Context::new(), DATABASE, create_database::Options::new()) |
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.
Is this idiomatic to call new()
, or should we implement the Default
trait? I see that used in quite a few samples, though they are pretty basic.
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.
Implementing default is probably a good idea for this struct. Other options structs won't have defaults since they might have required options. Creating a collection, for example, requires a partition key (at least for now).
@heaths I think this is good to go. |
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.
Great work. 👍
I am skeptical about the naming change that made the code much more verbose. Are we sure we want to follow this standard? We will end up with GetContainerACLOptions
, GetContainerACLResponse
etc... that are painful to read.
What about the previous standard (use modules)? I think is more idiomatic in Rust.
@MindFlavor I was also concerned about verbosity, but I'd like to give this way a try. It doesn't seem too bad right now. |
This is an example of how to move a certain operation to the new pipeline based architecture.
We want to do this for all operations.