Move more things into `codegen-core` #1762

david-perez · 2022-09-22T18:34:17Z

This moves most things from codegen-client that should be in
codegen-core into codegen-core.

This is a continuation of the efforts started in #1697, #1730.

Testing

No generated diff should be observed.

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

…hings-into-codegen-core

This moves most things from `codegen-client` that should be in `codegen-core` into `codegen-core`. This is a continuation of the efforts started in #1697, #1730.

github-actions · 2022-09-22T18:45:42Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

jdisanti

Nice work! Love seeing this happen.

It looks like none of the unit tests were moved? Most of them require testutil to move, which is not so easy. But it looks like you have enough critical mass of classes moved that testutil should actually move over successfully now. I would definitely prefer the tests move along with the core classes.

jdisanti · 2022-09-22T23:00:21Z

.../software/amazon/smithy/rust/codegen/core/smithy/transformers/RemoveEventStreamOperations.kt

@@ -3,13 +3,13 @@
 * SPDX-License-Identifier: Apache-2.0
 */

-package software.amazon.smithy.rust.codegen.client.smithy.transformers
+package software.amazon.smithy.rust.codegen.core.smithy.transformers


I'm surprised to see RemoveEventStreamOperations used by the server. Was this intentional? It requires allow listing service module names through the smithy-build.json.

I was surprised too. I've opened #1764; once that's merged, I'll keep the transformer in codegen-client in this PR.

#1764 was merged; I've moved RemoveEventStreamOperations back to the client in 67dc987.

jdisanti · 2022-09-22T23:05:20Z

...ain/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/ClientProtocolLoader.kt

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package software.amazon.smithy.rust.codegen.core.smithy.protocols


This file should be renamed to ProtocolLoader.kt

Good catch! Done in Rename protocol loader in codegen-core from ClientProtocolLoader ….

jdisanti · 2022-09-22T23:06:51Z

.../main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/GenericsGenerator.kt

@@ -3,12 +3,12 @@
 * SPDX-License-Identifier: Apache-2.0
 */

-package software.amazon.smithy.rust.codegen.client.smithy.generators
+package software.amazon.smithy.rust.codegen.core.smithy.generators


I think this class belongs in rustlang with the code that uses it.

Done in Move GenericsGenerator into rustlang and rename it to RustGenerics. I also renamed it to RustGenerics.

jdisanti · 2022-09-22T23:07:46Z

...-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/RustReservedWords.kt

@@ -3,7 +3,7 @@
 * SPDX-License-Identifier: Apache-2.0
 */

-package software.amazon.smithy.rust.codegen.client.rustlang
+package software.amazon.smithy.rust.codegen.core.rustlang


I think RustReservedWords should stay in client for now. It needs to be broken up into multiple composed classes since right now it has client-specific logic in it to handle the Unknown enum variants.

I agree we should refactor that bit out. However, I can't keep it in codegen-client, since BuilderGenerator, that is codegen-core depends on it, and we can't have core depending on client. We should eventually put BuilderGenerator in codegen-client anyway, since in #1342 the server project has its own builder generators. However, if I do so now in this PR, several things break, because the extension functions defined in BuilderGenerator.kt (StructureShape.builderSymbol, RuntimeConfig.operationBuildError(), MemberShape.setterName()...) are ingrained throughout codegen-core. Untangling all these is related to the CodegenTarget and reducing branching cleanup, that we should tackle separately.

I opened #1766 to track this.

jdisanti · 2022-09-22T23:10:01Z

...src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/UnionGenerator.kt

+import software.amazon.smithy.rust.codegen.core.rustlang.rustBlock
+import software.amazon.smithy.rust.codegen.core.smithy.CodegenTarget
+import software.amazon.smithy.rust.codegen.core.smithy.expectRustMetadata
+import software.amazon.smithy.rust.codegen.core.smithy.renamedFrom
 import software.amazon.smithy.rust.codegen.core.util.toSnakeCase

 fun CodegenTarget.renderUnknownVariant() = when (this) {


This function shouldn't get copied into core.

See my comment here #1762 (comment).

jdisanti · 2022-09-22T23:14:50Z

codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/CodegenTarget.kt

@@ -3,7 +3,7 @@
 * SPDX-License-Identifier: Apache-2.0
 */

-package software.amazon.smithy.rust.codegen.client.smithy.generators
+package software.amazon.smithy.rust.codegen.core.smithy


I don't think core should have the concept of CodegenTarget. Anything that depends on should be kept in client so that it can be individually refactored into core with the target if/else cases removed.

I agree with this sentiment in the general case, but:

Untangling the branching on CodegenTarget is diffcult; and

I'm actually not sure that removing the branching entirely is beneficial. In certain particular core generators, all you need is a quick and dirty if (target == ...) to get the job done. In some cases I think that introducing inheritance/composition/customization injections to get rid of the notion of CodegenTarget can be detrimental to code maintainability.

CodegenTarget is used by > 14 generators that this PR places in codegen-core. Keeping them in codegen-client entails bringing more things back. It's too ingrained. I think putting CodegenTarget in codegen-core is the sweet spot here.

I opened #1765 to track this.

jdisanti · 2022-09-22T23:15:19Z

...n-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/CoreCodegenContext.kt

@@ -3,12 +3,11 @@
 * SPDX-License-Identifier: Apache-2.0
 */

-package software.amazon.smithy.rust.codegen.client.smithy
+package software.amazon.smithy.rust.codegen.core.smithy


I think we should rename the Core- prefix out of these now that they're in the actual core module.

Agree. Done in Rename CoreCodegenContext as CodegenContext, since it's now in th….

…degenVisitor` This is a holdover from when the server subproject was started. We've never utilized this model transformer, nor will we have any use for it now, since event stream operations are supported in the server since #1479. See #1762 (comment)

…to `ProtocolLoader`

github-actions · 2022-09-23T11:23:46Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

…e `codegen-core` module

github-actions · 2022-09-23T16:48:45Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

…degenVisitor` (#1764) This is a holdover from when the server subproject was started. We've never utilized this model transformer, nor will we have any use for it now, since event stream operations are supported in the server since #1479. See #1762 (comment).

…hings-into-codegen-core

github-actions · 2022-09-26T10:42:58Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

…hings-into-codegen-core

github-actions · 2022-09-27T10:44:20Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python

A new doc preview is ready to view.

david-perez added 11 commits September 22, 2022 18:25

Move things into codegen-core --- sa, ca work

0d637c4

server-unit-tests work

8bdb97b

client-unit-tests work

a0da58e

sdk works

0ff4694

codegen-server-test:python works

ddaef40

./gradlew ktlintFormat

9d6ad32

Address TODOs

a21fe4d

./gradlew ktlintFormat

ba76e98

Copyright header

a359e97

Merge remote-tracking branch 'upstream/main' into davidpz/move-more-t…

452593e

…hings-into-codegen-core

Move more things into codegen-core

953ae53

This moves most things from `codegen-client` that should be in `codegen-core` into `codegen-core`. This is a continuation of the efforts started in #1697, #1730.

david-perez added needs-sdk-review needs-server-review labels Sep 22, 2022

david-perez requested review from a team as code owners September 22, 2022 18:34

jdisanti reviewed Sep 22, 2022

View reviewed changes

david-perez added 2 commits September 23, 2022 12:50

Fix Python Server unit tests

6ea6f54

Fix AWS SDK Kotlin unit tests

74a5278

david-perez mentioned this pull request Sep 23, 2022

Remove RemoveEventStreamOperations model transformer from ServerCodegenVisitor #1764

Merged

Rename protocol loader in codegen-core from ClientProtocolLoader …

0876167

…to `ProtocolLoader`

david-perez added 3 commits September 23, 2022 14:40

Move GenericsGenerator into rustlang and rename it to RustGenerics

d6385d9

Rename CoreCodegenContext as CodegenContext, since it's now in th…

90d250d

…e `codegen-core` module

./gradlew ktlintFormat

66acea3

This was referenced Sep 23, 2022

Reduce usage of CodegenTarget #1765

Open

Split RustReservedWords into composed classes #1766

Closed

import kotlin.streams.toList

c34dc46

hlbarber approved these changes Sep 23, 2022

View reviewed changes

david-perez removed the needs-server-review label Sep 26, 2022

david-perez added 2 commits September 26, 2022 12:10

Merge remote-tracking branch 'upstream/main' into davidpz/move-more-t…

67dc987

…hings-into-codegen-core

./gradlew ktlintFormat

36c3cdf

jdisanti approved these changes Sep 26, 2022

View reviewed changes

jdisanti removed the needs-sdk-review label Sep 26, 2022

Merge remote-tracking branch 'upstream/main' into davidpz/move-more-t…

667aaef

…hings-into-codegen-core

david-perez merged commit 5b7aa7b into main Sep 27, 2022

david-perez deleted the davidpz/move-more-things-into-codegen-core branch September 27, 2022 12:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move more things into `codegen-core` #1762

Move more things into `codegen-core` #1762

david-perez commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

jdisanti left a comment

jdisanti Sep 22, 2022

david-perez Sep 23, 2022

david-perez Sep 26, 2022

jdisanti Sep 22, 2022

david-perez Sep 23, 2022

jdisanti Sep 22, 2022

david-perez Sep 23, 2022

jdisanti Sep 22, 2022

david-perez Sep 23, 2022

david-perez Sep 23, 2022

jdisanti Sep 22, 2022

david-perez Sep 23, 2022

jdisanti Sep 22, 2022

david-perez Sep 23, 2022

david-perez Sep 23, 2022

jdisanti Sep 22, 2022

david-perez Sep 23, 2022

github-actions bot commented Sep 23, 2022

github-actions bot commented Sep 23, 2022

github-actions bot commented Sep 26, 2022

github-actions bot commented Sep 27, 2022

Move more things into codegen-core #1762

Move more things into codegen-core #1762

Conversation

david-perez commented Sep 22, 2022

Testing

github-actions bot commented Sep 22, 2022

jdisanti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 23, 2022

github-actions bot commented Sep 23, 2022

github-actions bot commented Sep 26, 2022

github-actions bot commented Sep 27, 2022

Move more things into `codegen-core` #1762

Move more things into `codegen-core` #1762