-
Notifications
You must be signed in to change notification settings - Fork 664
Implement SpacetimeType for Result<T, E> #3790
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
Conversation
70ca89a to
4324cd7
Compare
|
I'll build out an attached PR to add the new functionality for the Unreal SDK. |
5a99c91 to
cf9dd2f
Compare
JasonAtClockwork
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is updates to the /modules/sdk-test-cs but this won't be enough testing for the C# side. We'll need to add tests into the /sdks/csharp/examples~/regression-tests like for UUID to make sure this works end-to-end.
@rekhoff - I'll also want your eyes on this one when you can for the C# side
gefjon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with the Rust parts of this PR. We'll still need review from the other languages' maintainers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The C# side looks good. I did some local testing of end-to-end behavior, and things look good.
coolreader18
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me, except for my one comment.
604c452 to
c7b1f53
Compare
32ebe84 to
bd8dba7
Compare
bd8dba7 to
d5bd9e0
Compare
JasonAtClockwork
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a couple comments but otherwise this looks like a solid fix for tests on the C# side!
...examples~/regression-tests/client/module_bindings/Procedures/InsertWithTxRollbackResult.g.cs
Show resolved
Hide resolved
a85626f to
370e005
Compare
370e005 to
6898dc4
Compare
Signed-off-by: Ryan <r.ekhoff@clockworklabs.io>
Signed-off-by: Ryan <r.ekhoff@clockworklabs.io>
# Description of Changes Closes #3673 *NOTE*: C++ part will be in another PR # Expected complexity level and risk 2 Adding a new type touch everywhere # Testing - [x] Adding smoke and unit test --------- Signed-off-by: Ryan <r.ekhoff@clockworklabs.io> Co-authored-by: rekhoff <r.ekhoff@clockworklabs.io> Co-authored-by: Phoebe Goldman <phoebe@goldman-tribe.org> Co-authored-by: Jason Larabie <jason@clockworklabs.io>
Description of Changes
Closes #3673
NOTE: C++ part will be in another PR
Expected complexity level and risk
2
Adding a new type touch everywhere
Testing