Skip to content
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

Merged
merged 60 commits into from
Jun 14, 2023

Conversation

thomas-k-cameron
Copy link
Contributor

@thomas-k-cameron thomas-k-cameron commented Apr 27, 2023

Motivation and Context

This is a child PR of #2615.

Description

  • Adds 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.

/// client.$fnName().set_fields(&deserialized_parameters).send().await
/// };
/// ```
pub fn set_fields(mut self, data: $inputBuilderType) -> Self {
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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
    }
}

Copy link
Contributor Author

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.

Copy link
Collaborator

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

implBlock(symbolProvider.symbolForBuilder(input)) {
rustTemplate(
"""
##[#{AwsSdkUnstableAttribute}]
Copy link
Collaborator

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 {
Copy link
Collaborator

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

@thomas-k-cameron
Copy link
Contributor Author

Everything is fixed!

@jdisanti
Copy link
Collaborator

Kicked off PR bot so we can see a codegen diff: https://github.com/awslabs/smithy-rs/actions/runs/5259633320

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Collaborator

@jdisanti jdisanti left a 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!

@jdisanti jdisanti added this pull request to the merge queue Jun 13, 2023
@jdisanti jdisanti removed this pull request from the merge queue due to a manual request Jun 13, 2023
@jdisanti
Copy link
Collaborator

Actually, could you add an entry to the CHANGELOG.next.toml for both [[smithy-rs]] and [[aws-sdk-rust]]?

@thomas-k-cameron
Copy link
Contributor Author

I updated the changelog. Does it work?

@thomas-k-cameron thomas-k-cameron changed the title Add send_with method to fluent builders Add send_with method to fluent builders and add serde support to SDK geenrated datatypes Jun 14, 2023
@thomas-k-cameron thomas-k-cameron changed the title Add send_with method to fluent builders and add serde support to SDK geenrated datatypes Add send_with method to fluent builders Jun 14, 2023
CHANGELOG.next.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@jdisanti jdisanti left a 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!

@jdisanti jdisanti enabled auto-merge June 14, 2023 16:56
@jdisanti jdisanti added this pull request to the merge queue Jun 14, 2023
Merged via the queue into smithy-lang:main with commit 1e2c03c Jun 14, 2023
@thomas-k-cameron thomas-k-cameron deleted the RFC30/add-set-fields branch June 14, 2023 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants