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

fix: correctly codegen maps with enum keys #1052

Merged
merged 5 commits into from
Mar 15, 2024
Merged

fix: correctly codegen maps with enum keys #1052

merged 5 commits into from
Mar 15, 2024

Conversation

ianbotsf
Copy link
Contributor

Issue #

#1045

Description of changes

This change fixes the accidental generation of all map keys as strings when keys can also be string enums.

This change also adds a new flag to our KotlinSettings type used in smithy-build.json files called debug. When enabled, it will turn on stack frame emission in codegen which aids in codegen debug.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ianbotsf ianbotsf requested a review from a team as a code owner March 12, 2024 17:53

val sparseMapSymbol = provider.toSymbol(model.expectShape<MapShape>("${TestModelDefault.NAMESPACE}#MySparseMap"))

assertEquals("Map<String, Record?>", sparseMapSymbol.name)

// collections should contain a reference to the member type
assertEquals("Record", sparseMapSymbol.references[0].symbol.name)
val sparseRefNames = mapSymbol.references.map { it.symbol.fullName }
Copy link
Contributor

Choose a reason for hiding this comment

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

question: is this supposed to use sparseMapSymbol instead of mapSymbol?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is, good catch!


# Release date

This feature will ship with the **v1.0.80** release planned for **3/18/2024**.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we doing a minor version bump?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yep, we are. I guess this will ship with v1.1.0.

aajtodd pushed a commit that referenced this pull request Mar 14, 2024
Copy link

sonarcloud bot commented Mar 14, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
24.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

{
"id": "a7f82a33-11f1-4184-97af-ff713e922dfc",
"type": "bugfix",
"description": "⚠️ **IMPORTANT**: Fix codegen for map shapes which use string enums as map keys. See the Map Enum Keys breaking change announcement (link coming soon) for more details",
Copy link
Contributor

@0marperez 0marperez Mar 14, 2024

Choose a reason for hiding this comment

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

Looks good! I have a question, will this ship with the link or will it be added after?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will ship with the link. Now that the PR is approved, I'll remove the BREAKING-CHANGE.tmp.md file, post it as a GH discussion, update the changelog to include a link, and then merge the code to main.

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