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

Move ClientBuilderGenerator out of codegen-client #2159

Closed
wants to merge 22 commits into from

Conversation

jjant
Copy link
Contributor

@jjant jjant commented Jan 3, 2023

Motivation and Context

Closes #1970.

Description

Move a whole bunch of client-specific code from codegen-core to codegen-client.


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

@jjant jjant requested review from a team as code owners January 3, 2023 18:03
@jjant jjant marked this pull request as draft January 3, 2023 18:03
Comment on lines 234 to 239
{
"k1": { "map": {} },
"k2": { "map": { "k3": {} } },
"k3": { }
}
""",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ktlint complained here and below.

@jjant jjant marked this pull request as ready for review January 15, 2023 22:29
@github-actions
Copy link

github-actions bot commented Feb 7, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.

@smithy-lang smithy-lang deleted a comment from github-actions bot Feb 8, 2023
@smithy-lang smithy-lang deleted a comment from github-actions bot Feb 8, 2023
@smithy-lang smithy-lang deleted a comment from github-actions bot Feb 8, 2023
@github-actions
Copy link

github-actions bot commented Feb 8, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.

jdisanti
jdisanti previously approved these changes Feb 17, 2023
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.

This looks mostly good, just a couple comments.

My current work on crate reorganization is getting blocked on this PR, so I'm open to just merging it as is (once CI passes) and I can address the comments myself.

Comment on lines +96 to +99
interface BuilderGenerator {
fun render(writer: RustWriter)
fun renderConvenienceMethod(implBlock: RustWriter)
}
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 not a fan of this since it makes the builders inherit from unit test code, but it seems like this might go away due to #2342? Perhaps we merge that PR first and then delete this BuilderGenerator interface.

@@ -370,3 +376,23 @@ open class Instantiator(
else -> throw CodegenException("Unrecognized shape `$shape`")
}
}
fun StructureShape.builderSymbol(symbolProvider: RustSymbolProvider): Symbol {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add a symbolForBuilder(shape: Shape) to RustSymbolProvider instead of this approach (there are examples in there for operation error and event stream error symbols). The module location needs to be configurable for my crate reorganization work.

@jdisanti
Copy link
Collaborator

Hmmm... Digging into this more, I'm thinking we should actually keep a minimal BuilderGenerator in codegen-core simply due to the number of unit tests that require builders. The protocol tests should stay in codegen-core, or otherwise, codegen-core doesn't have adequate testing on its own.

@jdisanti jdisanti dismissed their stale review February 17, 2023 17:06

Revoking my approval

@Velfi
Copy link
Contributor

Velfi commented Jul 10, 2023

This PR has been sitting around for a while. Is it still relevant or can we close it?

@rcoh rcoh requested review from a team as code owners November 14, 2023 02:21
@jmklix jmklix closed this Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants