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

i1494 passing opts to attributevalue marshalling #1495

Merged
merged 3 commits into from
Jan 11, 2022
Merged

Conversation

radutopala
Copy link
Contributor

@radutopala radutopala commented Nov 13, 2021

Fixes #1494

Copy link
Contributor

@jasdel jasdel left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to create this PR @radutopala. Modifing the signatures of the Marshal methods would be a breaking change, and we could not release it.

Alternatively new Marshal methods could be defined with different names, e.g. MarshalWithOptions to take the functional options.

Also we'd probably want similar new functions for the Unmarshal set of functions.

@radutopala
Copy link
Contributor Author

Adding optFns ...func(*EncoderOptions) as a variadic arg means no breaking changes, one can call Marshall with or without passing any optFns. But if I'm missing something, please let me know.

@jasdel
Copy link
Contributor

jasdel commented Dec 4, 2021

Thanks for the feedback. Adding a variadic arg to a function call will not break existing callers, but since in Go functions can be types and variables, any application that creates a variable or struct member type matching the Marshal function signature would break if variadic arg were added. This is the reasoning behind suggesting this behavior would need to be new functions.

https://go.dev/play/p/c4yT8-DsHo9

@radutopala
Copy link
Contributor Author

@jasdel it's an extreme edge case, wondering who'd use this pattern with this specific func. But ok, better safe! I'll push some changes.

@radutopala
Copy link
Contributor Author

@jasdel branch updated, please CR / provide next steps.

Copy link
Contributor

@jasdel jasdel left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to put this PR together, and make the requested updates. I've added changelog notes to the PR. We'll get this update merged in and released in the SDK's next update.

jasdel and others added 3 commits January 11, 2022 08:54
Fixes the SDK's code generation to pin smithy-cli to $smithyVersion to
restrict the supported version.
@jasdel jasdel merged commit 07e5d3e into aws:main Jan 11, 2022
@radutopala radutopala deleted the i1494 branch January 11, 2022 19:16
jrichard8 pushed a commit to jrichard8/aws-sdk-go-v2 that referenced this pull request Feb 14, 2022
Fixes the SDK's code generation to pin smithy-cli to $smithyVersion to
restrict the supported version.

Co-authored-by: Jason Del Ponte <[email protected]>
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.

Pass KeyTag to dynamo attributevalue marshalling
2 participants