-
Notifications
You must be signed in to change notification settings - Fork 663
Fix fields named 'read' #2525
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 fields named 'read' #2525
Conversation
Makes sense. What do you think @jdetter? |
ff983f9 to
92eeb08
Compare
|
I don't super care about @kazimuth @joshua-spacetime does that sound good? |
|
@kazimuth Does this actually not have any API/ABI breaking changes? This doesn't require a C# SDK change? |
Add header comment CSharpier More comments
|
Needs to be rebased once #2710 is merged, that's higher priority |
Signed-off-by: rekhoff <r.ekhoff@clockworklabs.io>
|
Attempting to merge master with this PR ran into build issues, mostly due to conflicts with #2725 . |
…pacetimeDB into jgilles/fix-read
|
After additional fixes, all tests pass. I have tested the most recent changes against BitCraft and other minor tests, without issue. |
|
Tested this change against the "fixture" test and adding the fields to a quickstart-chat project. Both succeeded (after a minor complication error that is now patched) |
jdetter
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.
This looks fine to me, me and Ryan have gone over these changes together + he has added a test to make sure the fields are working correctly. Thanks Ryan 👍
Description of Changes
Fixes https://github.com/orgs/clockworklabs/projects/22?pane=issue&itemId=102392974&issue=clockworklabs%7Ccom.clockworklabs.spacetimedbsdk%7C276
by renaming
internalstaticserializer fields so that they do not overlap with user-provided names.Note, however, that some field names still will not work:
ReadFields,WriteFields,Equals, andGetHashCode.This would require a separate fix since the error would happen in a different place. In this case we would need to change the name of the generated member to something like
ReadFields_orEquals_, which I'm not sure is a good idea.API and ABI breaking changes
N/A
Expected complexity level and risk
0
Testing
SDK tests and
dotnet-verifytests are passing.