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

Standardized module names and added module-test-cs based on module-test #2232

Merged
merged 1 commit into from
Feb 9, 2025

Conversation

cloutiertyler
Copy link
Contributor

Description of Changes

This does the following renames:

/modules/rust-wasm-test -> /modules/module-test
/modules/spacetimedb-quickstart-cs -> /modules/module-test-cs

Additionally, I've removed /modules/spacetimedb-quickstart-cs and copied the contents into /modules/module-test taking care to update references in the tests.

Then I implemented the missing functionality in /modules/module-test-cs to make it as equivalent as possible to /modules/module-test, although it is not possible to match functionality as several small things are missing from the C# module implementation.

API and ABI breaking changes

No breaking changes.

Expected complexity level and risk

1.5 In principle only renames, but could potentially break testing.

Testing

Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!

  • I ran dotnet build on /modules/module-test-cs
  • I ran cargo test --all

Copy link
Collaborator

@joshua-spacetime joshua-spacetime left a comment

Choose a reason for hiding this comment

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

Looks as expected. I've noted the disparities you've highlighted so that we can create tickets for them.

modules/module-test-cs/Lib.cs Show resolved Hide resolved
modules/module-test-cs/Lib.cs Show resolved Hide resolved
modules/module-test-cs/Lib.cs Show resolved Hide resolved
modules/module-test/src/lib.rs Show resolved Hide resolved
@cloutiertyler cloutiertyler added this pull request to the merge queue Feb 9, 2025
Merged via the queue into master with commit e54a285 Feb 9, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants