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

Break up RustCodegenDecorator #2099

Merged
merged 4 commits into from
Dec 14, 2022
Merged

Conversation

jdisanti
Copy link
Collaborator

@jdisanti jdisanti commented Dec 14, 2022

Motivation and Context

This change creates ClientCodegenDecorator and
ServerCodegenDecorator in codegen-client and codegen-server respectively to replace RustCodegenDecorator. Client/server equivalents are created to replace CombinedCodegenDecorator as well.

This eliminates the need for the supportsCodegenContext method since the decorator interface is no longer generic, so the ServiceLoader is now powerful enough to differentiate.

The largest benefit, however, is that now clients and servers can have separate customizations.

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or 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.

This change creates `ClientCodegenDecorator` and
`ServerCodegenDecorator` in `codegen-client` and `codegen-server`
respectively to replace `RustCodegenDecorator`. Client/server
equivalents are created to replace `CombinedCodegenDecorator` as well.

This eliminates the need for the `supportsCodegenContext` method since
the decorator interface is no longer generic, so the `ServiceLoader` is
now powerful enough to differentiate.

The largest benefit, however, is that now clients and servers can have
separate customizations.
@jdisanti jdisanti force-pushed the jdisanti-split-codegen-decorator branch from 79db806 to d74997d Compare December 14, 2022 00:56
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Contributor

@crisidev crisidev left a comment

Choose a reason for hiding this comment

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

Looks great!

Comment on lines +153 to +171
companion object {
fun fromClasspath(
context: PluginContext,
vararg extras: ClientCodegenDecorator,
logger: Logger = Logger.getLogger("RustClientCodegenSPILoader"),
): CombinedClientCodegenDecorator {
val decorators = ServiceLoader.load(
ClientCodegenDecorator::class.java,
context.pluginClassLoader.orElse(ClientCodegenDecorator::class.java.classLoader),
)

val filteredDecorators = decorators.asSequence()
.onEach { logger.info("Discovered Codegen Decorator: ${it.javaClass.name}") }
.filter { it.classpathDiscoverable() }
.onEach { logger.info("Adding Codegen Decorator: ${it.javaClass.name}") }
.toList()
return CombinedClientCodegenDecorator(filteredDecorators + extras)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonderful, this is much more readable than what we had before with generics!

* AWS services. A different downstream customer may wish to add a different set of derive
* attributes to the generated classes.
*/
interface ServerCodegenDecorator {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's great that now the interface for the Server Codegen is smaller and completely customizable without interfering with the Client..

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—the only issue is it prevents the integration test helper from being usable for the serve . It would be nice if we could unit those but probably not mission critical

Comment on lines +15 to +18
references = ["smithy-rs#2099"]
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client"}
author = "jdisanti"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will break a ton of people...I wonder if we should have a backwards compatibility shim

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti merged commit 5970ef7 into main Dec 14, 2022
@jdisanti jdisanti deleted the jdisanti-split-codegen-decorator branch December 14, 2022 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client server Rust server SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants