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

Check if updating usage of EnumTrait to EnumShape improves code generated by smithy-rs #1671

Open
Velfi opened this issue Aug 25, 2022 · 3 comments
Assignees
Labels
good first issue Good for newcomers refactoring Changes that do not affect our users, mostly focused on maintainability

Comments

@Velfi
Copy link
Contributor

Velfi commented Aug 25, 2022

We currently have a lot of usages of if (shape.hasTrait<EnumTrait>()) { that may need to be converted to if (shape is EnumShape) {. Convert them and then verify that this has improved the generated code (i.e. if it generates less code or clearer code.)

@Velfi Velfi added the good first issue Good for newcomers label Aug 25, 2022
@jdisanti
Copy link
Collaborator

It looks like Smithy's ModelTransformer has a changeStringEnumsToEnumShapes transform, which would allow us to migrate the codegen to only look at EnumShape and stop using EnumTrait entirely. We could just throw this into the CodegenVisitor's baselineTransform.

@david-perez
Copy link
Contributor

There's also no behavioral change with regards to when invalid enum values are rejected in the server request deserialization path, so I think we should be good applying that model transform everywhere and not affect semantics, regardless of whether we're generating an IDL v1 or v2 model.

@jdisanti
Copy link
Collaborator

Confirmed with the Smithy folks that we should model transform from EnumTrait to EnumShape, and then refactor our EnumGenerator to work exclusively off the EnumShape.

82marbag added a commit that referenced this issue Nov 1, 2022
Deprecate EnumTrait and migrate to EnumShape.

Closes #1671

Signed-off-by: Daniele Ahmed <[email protected]>
@82marbag 82marbag mentioned this issue Nov 1, 2022
2 tasks
82marbag added a commit that referenced this issue Nov 1, 2022
Deprecate EnumTrait and migrate to EnumShape.

Closes #1671

Signed-off-by: Daniele Ahmed <[email protected]>
@82marbag 82marbag self-assigned this Nov 2, 2022
@jdisanti jdisanti added the refactoring Changes that do not affect our users, mostly focused on maintainability label Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers refactoring Changes that do not affect our users, mostly focused on maintainability
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants