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

Eliminate SDK client generics #1132

Merged
merged 9 commits into from
Feb 1, 2022
Merged

Conversation

jdisanti
Copy link
Collaborator

@jdisanti jdisanti commented Jan 28, 2022

Motivation and Context

The motivation here is to simplify implementation of waiters by reducing the amount of generic bounds required to implement them. The connector and middleware can still be customized since they are dynamic.

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

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

@jdisanti jdisanti requested a review from a team as a code owner January 28, 2022 21:03
@github-actions
Copy link

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

@github-actions
Copy link

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

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! This is huge IMO.

@@ -125,7 +129,7 @@ class PaginatorGenerator private constructor(
builder: #{Builder}
}

impl${generics.inst} ${paginatorName}${generics.inst} where #{bounds:W} {
impl${generics.inst} ${paginatorName}${generics.inst} #{bounds:W} {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: small inconsistency that one includes the where and the other does not

Comment on lines +71 to +72
/** Bounds for generated `send()` functions */
override fun sendBounds(input: Symbol, output: Symbol, error: RuntimeType): Writable = writable { }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm really surprised this compiles! It generates fn send(...) where { body }. Probably worth including the where in the bounds just so we generate a slightly more normal looking code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wow, good catch! I'm also surprised 😅

Comment on lines +141 to +142
.connector(#{DynConnector}::new(conn))
.middleware(#{DynMiddleware}::new(#{Middleware}::new()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could consider sticking these types into the client generics for a little DRY but not sure if it's worth it


[[aws-sdk-rust]]
message = """
`Client` no longer has generics, and now uses `DynConnector` and `DynMiddleware` to allow for connector/middleware customization.
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you update the changelog message to clarify that this is all generated service clients? Might also be worth calling out that this simplifies the code and docs.

The only other call out might be custom retry policies? Not sure.

@rcoh
Copy link
Collaborator

rcoh commented Jan 31, 2022

LGTM!

@github-actions
Copy link

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

@jdisanti jdisanti force-pushed the jdisanti-eliminate-sdk-client-generics branch from 1129b1b to be12ae2 Compare January 31, 2022 19:16
@github-actions
Copy link

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

Comment on lines +131 to +136
```rust
fn some_function(conn: DynConnector) -> Result<()> {
// ...
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

I felt a great disturbance in the Force, as if millions of voices where clauses suddenly cried out in terror and were suddenly silenced. I fear something terrible awesome has happened.

@github-actions
Copy link

github-actions bot commented Feb 1, 2022

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Feb 1, 2022

A new generated diff is ready to view.

@jdisanti jdisanti merged commit 4029afd into main Feb 1, 2022
@jdisanti jdisanti deleted the jdisanti-eliminate-sdk-client-generics branch February 1, 2022 21:52
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.

3 participants