Skip to content

Conversation

@wooj2
Copy link
Contributor

@wooj2 wooj2 commented Jun 29, 2021

Corresponding:
awslabs/aws-sdk-swift#150

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

@wooj2 wooj2 marked this pull request as ready for review June 30, 2021 15:11
@wooj2 wooj2 requested a review from kneekey23 June 30, 2021 15:11
Copy link
Contributor

@kneekey23 kneekey23 left a comment

Choose a reason for hiding this comment

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

Everything LGTM just had one question but feel free to just respond and ship if it's not a concern!

val shape = model.expectShape(member.target.toShapeId())
val expectedMemberName = "$expected.${symbolProvider.toMemberName(member)}"
val actualMemberName = "$actual.${symbolProvider.toMemberName(member)}"
if (member.isStructureShape) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this if statement even doing anything? If a member is a struct shape don't we want separate asserts for each prop anyways where it doesn't matter if it's a struct? What if the struct has a double with a nan value? Might be missing something here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, it was a copy and paste of the original implementation.

For now it seems to be working because these structs probably don't seem to have nested members that contain nan's. In the event that we start having protocol tests that do this, protocol codegen tests will fail, and then we'll need to update our logic at that time. Sound like a plan?

@wooj2 wooj2 merged commit 97d9953 into chore/smithyupdate Jun 30, 2021
@wooj2 wooj2 deleted the chore/smithyupdate-cmp branch June 30, 2021 17:34
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.

2 participants