Skip to content

Conversation

@rekhoff
Copy link
Contributor

@rekhoff rekhoff commented Nov 21, 2025

Description of Changes

Implements the C# module bindings for Procedures (#3510)

API and ABI breaking changes

None

Expected complexity level and risk

2

Testing

  • Locally tested against existing C# Procedures regression test client (minus the Transaction and HTTP portions, as those are not in this change).

@rekhoff rekhoff self-assigned this Nov 21, 2025
@rekhoff rekhoff marked this pull request as ready for review November 24, 2025 22:15
Copy link
Contributor

@JasonAtClockwork JasonAtClockwork left a 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.

Copy link
Contributor

@JasonAtClockwork JasonAtClockwork left a 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.

@rekhoff rekhoff changed the title Initial addition of procedures in csharp module bindings C# module bindings for Procedures Nov 25, 2025
@rekhoff
Copy link
Contributor Author

rekhoff commented Nov 25, 2025

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 procedure-client into the existing client regression test. I'll add the test suggestions to the list to implement as part of the other procedure pieces implementations.

@rekhoff rekhoff added this pull request to the merge queue Nov 25, 2025
Merged via the queue into master with commit 60e4a64 Nov 25, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants