-
Notifications
You must be signed in to change notification settings - Fork 660
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
| /// <summary> | ||
| /// Get the name of the BSATN struct for this type. | ||
| /// </summary> | ||
| public virtual string to_bsatn_string() |
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 guessing this was added as the Result has two types and that was giving trouble with the BSATNName property directly?
Two notes on this, this should be ToBSATNString() or something like it to match PascalCase for the C# convention followed in all the other code. Second BSATNName was heavily used in CodeGen/Module.cs and doesn't seem to be updated in all locations. This will need review to make sure it won't cause problems for Result<> types used throughout the codegen.
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.
Yes, for things like:
SpacetimeDB.Result<SpacetimeDB.Sdk.Test.Module.EveryPrimitiveStruct, string>.BSATN<SpacetimeDB.Sdk.Test.Module.EveryPrimitiveStruct, string, SpacetimeDB.Sdk.Test.Module.EveryPrimitiveStruct.BSATN, SpacetimeDB.BSATN.String> rRW = new();But is not possible to just pass the BSATNName everywhere, I need to manually pick some places.
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.
| } | ||
|
|
||
| /// <summary> | ||
| /// A use of a Result<T, E> type. |
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 will confuse the XML parser. We should use the < and > versions of the characters in summary blocks.
| /// A use of a Result<T, E> type. | |
| /// A use of a Result<T, E> type. |
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.
| } | ||
|
|
||
| /// 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),
}
}cf9dd2f to
af676ab
Compare
af676ab to
604c452
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