-
Notifications
You must be signed in to change notification settings - Fork 649
C# module bindings for Procedures #3732
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
…ithub.com/clockworklabs/SpacetimeDB into rekhoff/procedures-csharp-module-bindings
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.
Reviewed the code and ran the regression tests with no troubles; all looks good to me. The only question I have is if we should be adding checks to Codegen.Tests/fixtures/diag/Lib.cs and Codegen.Tests/fixtures/server/Lib.cs? Or maybe add that once all the pieces are in?
One test that would be good is a procedure that returns a non-SpacetimeDB.Type. This should fail due to the ReturnType.BSATNName but would be good to track that.
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.
Ahh just realized the client-side tests are not there for regression, I've added one locally just to make sure procedures are calling but you'll want to mirror what the /regression-test/procedure-client - once the TX pieces are in place we can probably delete that entire runner too.
|
This are some great ideas for additional tests. I was intending on getting add a more robust set of tests once the other pieces are in place, and migrate the |
Description of Changes
Implements the C# module bindings for Procedures (#3510)
API and ABI breaking changes
None
Expected complexity level and risk
2
Testing