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

Do not generate public empty modules #1803

Merged
merged 8 commits into from
Oct 4, 2022
Merged

Conversation

LukeMathWalker
Copy link
Contributor

@LukeMathWalker LukeMathWalker commented Oct 3, 2022

Motivation and Context

When experimenting with smithy-rs I noticed that the generated server crate exposes two public empty modules - operation and operation_handler.

Description

Make sure that operation and operational_handler are private modules in the generated server crate, while keeping operation public for the generated client crate.

Testing

I verified the generated server and client code via ./gradlew codegen-client-test:test and ./gradlew codegen-server-test:test.

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.

…de since it does not contain any public type.

Tune the visibility of the 'operation' module based on the rendering context
@LukeMathWalker LukeMathWalker requested review from a team as code owners October 3, 2022 12:15
@github-actions
Copy link

github-actions bot commented Oct 3, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@LukeMathWalker LukeMathWalker marked this pull request as draft October 3, 2022 13:17
@@ -34,7 +35,7 @@ class CustomizableOperationGenerator(
private val smithyTypes = CargoDependency.SmithyTypes(runtimeConfig).asType()

fun render(crate: RustCrate) {
crate.withModule(RustModule.Operation) { writer ->
crate.withModule(RustModule.operation(Visibility.PUBLIC)) { writer ->
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 would have expected this to be enough to get the module to show up in lib.rs as pub mod operation, but apparently it wasn't - I had to add https://github.com/awslabs/smithy-rs/pull/1803/files#diff-f7da198414ffe3cefc591164d3b1bb41d67e22812d7ba3a86b8abd6d798cec49R87
I'd be curious to understand why.

Copy link
Collaborator

Choose a reason for hiding this comment

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

From reading the code, it definitely seems like it should emit the mod statement when it's missing from DefaultPublicModules.

@github-actions
Copy link

github-actions bot commented Oct 3, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@LukeMathWalker LukeMathWalker marked this pull request as ready for review October 3, 2022 14:05
@@ -175,7 +175,6 @@ open class RustCrate(
*/
val DefaultPublicModules = setOf(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a better approach is to move DefaultPublicModules into the client and server modules, since it doesn't seem like there's a good reason to share it in codegen-core. This would also allow us to tailor the module docs to the client/servers separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In 8f1b0a8 I removed DefaultPublicModules and gave client/server/test the responsibility to handle their own list of default public modules.
I haven't inlined the definitions of the various module in those lists, but happy to do so if you think that's useful at this stage.

@@ -34,7 +35,7 @@ class CustomizableOperationGenerator(
private val smithyTypes = CargoDependency.SmithyTypes(runtimeConfig).asType()

fun render(crate: RustCrate) {
crate.withModule(RustModule.Operation) { writer ->
crate.withModule(RustModule.operation(Visibility.PUBLIC)) { writer ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

From reading the code, it definitely seems like it should emit the mod statement when it's missing from DefaultPublicModules.

@github-actions
Copy link

github-actions bot commented Oct 4, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Oct 4, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@LukeMathWalker LukeMathWalker merged commit 917d0a6 into main Oct 4, 2022
@LukeMathWalker LukeMathWalker deleted the fix-public-empty-modules branch October 4, 2022 16:15
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