Skip to content

Feat: fetchCurrentDevice API Implementation #5075

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

Merged
merged 2 commits into from
Jul 8, 2024

Conversation

hahnandrew
Copy link
Contributor

@hahnandrew hahnandrew commented Jun 25, 2024

Description of changes:

Implemented fetchCurrentDevice API

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

@hahnandrew hahnandrew requested a review from a team as a code owner June 25, 2024 22:44
@hahnandrew hahnandrew self-assigned this Jun 25, 2024
Comment on lines +1029 to +1032
attributes: {
for (final attribute in attributes)
attribute.name: attribute.value ?? '',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Does this need a for loop? Does attributes have other properties?

Suggested change
attributes: {
for (final attribute in attributes)
attribute.name: attribute.value ?? '',
},
attributes: attributes,

Copy link
Contributor Author

@hahnandrew hahnandrew Jun 27, 2024

Choose a reason for hiding this comment

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

Q: Does this need a for loop?

Yes, it's for type conversion. At first glance it looks redundant, but the loop achieves two important things - converting null values to an empty string, and converting a BuiltList to Map

Note that attributes: attributes is type invalid. CognitoDevice types its attributes as map<string, string>, while the attributes from the getDeviceResponse.device (smithy-generatively) types its attributes as BuiltList<AttributeType>?.

Does attributes have other properties?

No

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks for adding that context.

Copy link
Contributor

@Equartey Equartey left a comment

Choose a reason for hiding this comment

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

LGTM

@hahnandrew hahnandrew merged commit 45202d0 into feat/fetchCurrentDevice Jul 8, 2024
77 checks passed
@hahnandrew hahnandrew deleted the feat/fetchCurrentDevice-impl branch August 2, 2024 20: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