Skip to content
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

Fix declaration order of ScheduleAt in C# #2007

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kazimuth
Copy link
Contributor

And add some tests to sdk-test-cs. This was causing ScheduleAt to not get recognized by the schema validation code.

These tests don't fully exercise scheduling/indexes yet, but at least they check if they compile & are accepted by the host.

Expected complexity level and risk

0

Testing

Added tests.

@RReverser
Copy link
Member

Hm this doesn't look right. It seems it's Rust that is wrong here as C# implemented the enum according to the spec: https://github.com/clockworklabs/SpacetimeDBPrivate/blob/592a34c0cb82d587843d2b2309b824e94c035d26/proposals/0005-timers.md?plain=1#L82

Or, I guess, we can do this PR, but then we need to change the spec as well.

new("Interval", new AlgebraicType.U64(default)),
new("Time", new AlgebraicType.U64(default)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW this is the only necessary reordering, the rest don't matter and can be safely reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured, but I thought we might as well update all of them for consistency.

@kazimuth
Copy link
Contributor Author

Ah whoops. I think it's probably easier to change the spec at this point as it's less of a breaking change.

@kazimuth
Copy link
Contributor Author

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.

2 participants