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 bug where string builtIns were not supported #2150

Merged
merged 1 commit into from
Jan 4, 2023
Merged

Conversation

rcoh
Copy link
Collaborator

@rcoh rcoh commented Dec 30, 2022

Motivation and Context

String builtIn parameters resulted in a compilation error

Description

Fix EP2 bug with builtIn resolution

Testing

  • EP2 UT

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK 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.

@rcoh rcoh force-pushed the default-string-builtins branch 2 times, most recently from 2922165 to ce27b3d Compare December 30, 2022 15:33
@rcoh rcoh marked this pull request as ready for review December 30, 2022 15:34
@rcoh rcoh requested review from a team as code owners December 30, 2022 15:34
@rcoh rcoh requested review from jjant and unexge December 30, 2022 15:34
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

rustBlockTemplate("#{Params}", "Params" to paramsStruct()) {
parameters.toList().forEach { parameter ->
rust("${parameter.memberName()}: self.${parameter.memberName()}")
parameter.default.orNull()?.also { default -> rust(".or(Some(${value(default)}))") }
parameter.default.orNull()?.also { default -> rust(".or_else(||Some(${value(default)}))") }
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: space

Suggested change
parameter.default.orNull()?.also { default -> rust(".or_else(||Some(${value(default)}))") }
parameter.default.orNull()?.also { default -> rust(".or_else(|| Some(${value(default)}))") }

@@ -255,10 +255,11 @@ internal class EndpointParamsGenerator(private val parameters: Parameters) {
"ParamsError" to paramsError(),
) {
val params = writable {
Attribute.Custom("allow(clippy::unnecessary_lazy_evaluations)").render(this)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

when it's a boolean value, the generated code isn't a function invocation

val node = this
return writable {
when (node) {
is StringNode -> rust("Some(${node.value.dq()}.to_string())")
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the fix? I fail to see it 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is an unrelated refactoring

@rcoh rcoh enabled auto-merge (squash) January 4, 2023 17:28
@github-actions
Copy link

github-actions bot commented Jan 4, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.

@rcoh rcoh merged commit 5392a66 into main Jan 4, 2023
@rcoh rcoh deleted the default-string-builtins branch January 4, 2023 18:01
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