-
Notifications
You must be signed in to change notification settings - Fork 190
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
Add send_with method to fluent builders #2652
Add send_with method to fluent builders #2652
Conversation
- Add serde related attributes
/// client.$fnName().set_fields(&deserialized_parameters).send().await | ||
/// }; | ||
/// ``` | ||
pub fn set_fields(mut self, data: $inputBuilderType) -> Self { |
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.
Would it be possible to go in the other direction and have an input take a client to form a fluent builder? Something like:
SomeInput::builder()
.some_field("foo")
.with_client(client) // returns the fluent builder corresponding to the SomeInput operation
.send()
.await
My reasoning is that set_fields
could be called after some fields are set, and it would overwrite those fields, which could be confusing. If the API is designed so that that's not possible, all the better.
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 think this is lot better. I'm not quite sure if you can do this though. Let me investigate.
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 implemented like this.
Example::builder().into_fluent_builder(client).send().await;
Example:
impl AddPermissionInputBuilder {
#[cfg(aws_sdk_unstable)]
/// Creates a fluent builder from this builder.
pub fn into_fluent_builder(self, client: &crate::Client) -> AddPermissionFluentBuilder {
let fluent_builder = client.add_permission();
fluent_builder.inner = self;
fluent_builder
}
}
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.
fn into_*
is used when it takes up the ownership to create a value as another datatype so I think this name is good.
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.
Talking to the team a bit more about this, would making this into a send_with
function be better? Something like:
SomeInput::builder()
.some_field("foo")
.send_with(&client)
.await
...software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerator.kt
Outdated
Show resolved
Hide resolved
implBlock(symbolProvider.symbolForBuilder(input)) { | ||
rustTemplate( | ||
""" | ||
##[#{AwsSdkUnstableAttribute}] |
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.
This function will be just generally useful, so I would say let's remove the unstable attribute from it. May as well keep the supporting machinery around for your other PRs though.
/// client.$fnName().set_fields(&deserialized_parameters).send().await | ||
/// }; | ||
/// ``` | ||
pub fn set_fields(mut self, data: $inputBuilderType) -> Self { |
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.
Talking to the team a bit more about this, would making this into a send_with
function be better? Something like:
SomeInput::builder()
.some_field("foo")
.send_with(&client)
.await
…egen/client/smithy/generators/client/FluentClientGenerator.kt Co-authored-by: John DiSanti <[email protected]>
Everything is fixed! |
Kicked off PR bot so we can see a codegen diff: https://github.com/awslabs/smithy-rs/actions/runs/5259633320 |
A new generated diff is ready to view.
A new doc preview is ready to view. |
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.
Looks good. Thank you for the contribution!
Actually, could you add an entry to the |
I updated the changelog. Does it work? |
send_with
method to fluent builderssend_with
method to fluent builders and add serde support to SDK geenrated datatypes
send_with
method to fluent builders and add serde support to SDK geenrated datatypesThere 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 made some minor touch ups to the changelog entries. Everything looks good, thank you!
Motivation and Context
This is a child PR of #2615.
Description
send_with
method to Fluent Builder.Prerequisite PRs
You can merge this first too reduce diffs.
Testing
NA
Checklist
NA
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.