-
Notifications
You must be signed in to change notification settings - Fork 663
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
base: master
Are you sure you want to change the base?
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.
crates/sats/src/algebraic_value.rs
Outdated
| } | ||
|
|
||
| /// Converts `self` into an `Result<AlgebraicValue, AlgebraicValue>`, if applicable. | ||
| pub fn into_result(self) -> Result<Self, Self> { |
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.
Shouldn't this be -> Result<Result<AlgebraicType, AlgebraicType>, Self>?
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.
Mm, this is like the option variant:
/// Converts `self` into an `Option<AlgebraicValue>`, if applicable.
pub fn into_option(self) -> Result<Option<Self>, Self> {
match self {
AlgebraicValue::Sum(sum_value) => match sum_value.tag {
0 => Ok(Some(*sum_value.value)),
1 => Ok(None),
_ => Err(AlgebraicValue::Sum(sum_value)),
},
_ => Err(self),
}
}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 think the equivalent would be:
pub fn into_result(self) -> Result<Result<Self, Self>, Self> {
match self {
AlgebraicValue::Sum(sum_value) => match sum_value.tag {
0 => Ok(Ok(*sum_value.value)),
1 => Ok(Err(*sum_value.value)),
_ => Err(self),
},
_ => Err(self),
}
}
This way, it's possible to distinguish between the case where self is a Result::Err (so you get Ok(Err(val)), and the case where self is not a Result at all (so you get Err(self)). Your version sends both Result::Err and non-result values to Err.
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!
| } | ||
|
|
||
| public InsertWithTxRollbackResult() | ||
| { |
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.
We'll either want to have required keyword added to the generated use of SpacetimeDB.Result<> or add a default in the empty constructor like:
this.Value = default!@rekhoff - Do you have any preference, without this devs will have ugly warnings using Result<> in C#?
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.
This is the warning devs will see:
warning CS8618: Non-nullable field 'Msg' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the field as nullable.
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.
No strong preference here, but I think I lean towards the default empty constructor. If a developer doesn't actually care about the results, marking it as required could be kind of annoying to them.
a85626f to
370e005
Compare
370e005 to
6898dc4
Compare
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