Skip to content

i1494 passing opts to attributevalue marshalling#1495

Merged
jasdel merged 3 commits intoaws:mainfrom
radutopala:i1494
Jan 11, 2022
Merged

i1494 passing opts to attributevalue marshalling#1495
jasdel merged 3 commits intoaws:mainfrom
radutopala:i1494

Conversation

@radutopala
Copy link
Copy Markdown
Contributor

@radutopala radutopala commented Nov 13, 2021 •
edited
Loading

Fixes #1494

jasdel
jasdel suggested changes Dec 3, 2021
View reviewed changes
Copy link
Copy Markdown
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.

@jasdel jasdel mentioned this pull request Dec 3, 2021
Closed
3 tasks
@radutopala
Copy link
Copy Markdown
Contributor Author

radutopala commented Dec 4, 2021

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
Copy Markdown
Contributor

jasdel commented Dec 4, 2021 •
edited
Loading

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
Copy Markdown
Contributor Author

radutopala commented Dec 4, 2021

@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
Copy Markdown
Contributor Author

radutopala commented Dec 4, 2021

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

jasdel
jasdel approved these changes Jan 10, 2022
View reviewed changes
Copy link
Copy Markdown
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
@jasdel
codegen: Adds smithy-cli dependency to prevent auto use of smithy-cli
4fd2d2e 
Fixes the SDK's code generation to pin smithy-cli to $smithyVersion to
restrict the supported version.
@radutopala @jasdel
i1494 passing opts to dynamo attributevalue marshalling
2ecb204
@jasdel
add changelog
e5f9414
@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
@radutopala @jasdel @jrichardpfs
i1494 passing opts to attributevalue marshalling (aws#1495)
80ec801 
Fixes the SDK's code generation to pin smithy-cli to $smithyVersion to
restrict the supported version.

Co-authored-by: Jason Del Ponte <961963+jasdel@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jasdel jasdel jasdel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Pass KeyTag to dynamo attributevalue marshalling

2 participants

@radutopala @jasdel