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

Support @sparse constrained map shapes and list shapes #2213

Merged
merged 5 commits into from Jan 17, 2023

Conversation

david-perez
Copy link
Contributor

Turns out we've never supported them, neither directly constrained nor
with constrained members, because of a lack of tests. Yet another data
point to prioritize working on code-generating constraints.smithy (see
#2101).

The implementation is simple: we just need to call the symbol provider
on the member symbols instead of on the target symbols so we get
Option<T> list members / map values if applicable, and handle the
wrapper when converting between unconstrained and constrained types with
help from match and Option<T>::map.


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

Turns out we've never supported them, neither directly constrained nor
with constrained members, because of a lack of tests. Yet another data
point to prioritize working on code-generating `constraints.smithy` (see
#2101).

The implementation is simple: we just need to call the symbol provider
on the member symbols instead of on the target symbols so we get
`Option<T>` list members / map values if applicable, and handle the
wrapper when converting between unconstrained and constrained types with
help from `match` and `Option<T>::map`.
@david-perez david-perez added the bug Something isn't working label Jan 13, 2023
@david-perez david-perez added this to the Server GA milestone Jan 13, 2023
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

}
""",
"ConstrainedValueSymbol" to constrainedValueSymbol,
"Epilogue" to epilogueWritable,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Epilogue" to epilogueWritable,

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look to be used here in this block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b4ae60f.

Copy link
Contributor

@crisidev crisidev left a comment

Choose a reason for hiding this comment

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

Just a small comment on a value that looks to be unused. LGTM.

}
""",
"ConstrainedValueSymbol" to constrainedValueSymbol,
"Epilogue" to epilogueWritable,
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look to be used here in this block

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@david-perez david-perez enabled auto-merge (squash) January 17, 2023 12:59
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@david-perez david-perez merged commit 9f90517 into main Jan 17, 2023
@david-perez david-perez deleted the davidpz/support-sparse-constrained-maps-and-lists branch January 17, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working constraint-traits server Rust server SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants