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

Consolidate fluent client builder implementations #668

Merged
merged 10 commits into from
Aug 27, 2021

Conversation

jdisanti
Copy link
Collaborator

Motivation and Context

The AWS fluent client implementation is currently separate from the generic Smithy fluent client implementation. The aim of this PR is to consolidate them so that changes made to one take effect in the other.

Testing

  • Verified examples still compile and manually tested the S3 ListBuckets example

Checklist

  • I have updated the CHANGELOG

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jdisanti
Copy link
Collaborator Author

It looks like this PR makes passing around an individual operation's fluent builder more difficult. To illustrate how, here is a before/after snapshot from the Dynamo movies example:

Before:

fn create_table(
    client: &Client,
    table_name: &str,
) -> CreateTable {

After:

fn create_table(
    client: &Client,
    table_name: &str,
) -> CreateTable<DynConnector, AwsMiddleware, Standard> {

The PR currently has the clients arranged as follows:

crate
└── Client = crate::client::Client<DynConnector, AwsMiddleware, Standard>
crate::client
├── Client<C, M, R>
└── fluent_builders
    ├── SomeOperation<C, M, R>
    └── AnotherOperation<C, M, R>

I think we will need create a client::generic module and create a type alias for every operation's builder, as shown below:

crate
└── Client = crate::client::Client
crate::client
├── Client = Client<DynConnector, AwsMiddleware, Standard>
└── fluent_builders
    ├── SomeOperation = crate::client::generic::fluent_builders::SomeOperation<DynConnector, AwsMiddleware, Standard>
    └── AnotherOperation = crate::client::generic::fluent_builders::AnotherOperation<DynConnector, AwsMiddleware, Standard>
crate::client::generic
├── Client<C, M, R>
└── fluent_builders
    ├── SomeOperation<C, M, R>
    └── AnotherOperation<C, M, R>

@rcoh - What are your thoughts on this?

@rcoh
Copy link
Collaborator

rcoh commented Aug 25, 2021

can we conditionally generate default type parameters for the AWS case? Maybe we can have a defaultType map passed into the fluent builder core, something like:

data class DefaultTypes(connector: RuntimeType?, middleware: RuntimeType?, retry: RuntimeType?)

Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is huge and sorely needed. You can also undo my refactoring to create FluentClientCore when this lands.

@jdisanti
Copy link
Collaborator Author

can we conditionally generate default type parameters for the AWS case? Maybe we can have a defaultType map passed into the fluent builder core

That's a really good idea, I like it! Will give it a try.

@jdisanti jdisanti requested a review from rcoh August 25, 2021 23:38
Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one cleanup inline


val AwsMiddleware = RuntimeType("AwsMiddleware", awsHyperDep, "aws_hyper")
val DynConnector = RuntimeType("DynConnector", smithyClientDep, "smithy_client::erase")
val Standard = RuntimeType("Standard", smithyClientDep, "smithy_client::retry")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: probably clearer to have this be the retry module so that it's #{retry}::Standard which is much clearer than just Standard

@jdisanti jdisanti merged commit f9d3f28 into smithy-lang:main Aug 27, 2021
@jdisanti jdisanti deleted the consolidate-client-builders branch August 27, 2021 17:13
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