feat(dojo-core): add support for making multiple model pointers #2940
Conversation
WalkthroughOhayo, sensei! This pull request introduces enhancements to the model pointer functionality in the Dojo framework. The changes primarily focus on expanding the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/dojo/core/src/model/model.cairo (2)
164-170: Consider making span dereferencing more explicit, sensei.While the implementation is correct, the
*keydereferencing could be made more explicit for better readability.- ptrs.append(Self::ptr_from_serialized_keys(*key)); + let key_span = *key; + ptrs.append(Self::ptr_from_serialized_keys(key_span));
172-178: Apply consistent dereferencing style, sensei.Similar to the previous suggestion, consider making the dereferencing more explicit.
- ptrs.append(Self::ptr_from_id(*entity_id)); + let id = *entity_id; + ptrs.append(Self::ptr_from_id(id));crates/dojo/core-cairo-test/src/tests/model/model.cairo (1)
208-221: Consider simplifying the type parameter formatting, sensei.While the test coverage is excellent, the type parameter formatting for
Model::<Foo>spans multiple lines unnecessarily.- let ptrs_c = Model::< - Foo, - >::ptrs_from_serialized_keys( + let ptrs_c = Model::<Foo>::ptrs_from_serialized_keys(
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/dojo/core-cairo-test/src/tests/model/model.cairo(2 hunks)crates/dojo/core/src/model/model.cairo(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: docs
- GitHub Check: ensure-wasm
🔇 Additional comments (5)
crates/dojo/core/src/model/model.cairo (4)
Line range hint
8-12: Ohayo! Clean and well-documented struct implementation, sensei!The
ModelPtr<M>struct is well-designed with appropriate trait derivations and clear documentation.
67-72: Excellent addition of batch processing methods, sensei!The new methods for generating multiple model pointers follow a consistent pattern and are well-documented.
156-162: Clean implementation of batch key processing, sensei!The implementation efficiently reuses
entity_id_from_keysand follows the established pattern.
62-62: Verify impact of Drop bound addition.The addition of the
Dropbound toptr_from_keysmight affect existing implementations.✅ Verification successful
Ohayo sensei!
Drop bound addition is safe to proceed ✨
The verification shows all existing implementations and usages of
ptr_from_keysare compatible with the newDropbound. Found usages in tests and examples already work with types that implementDrop.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing implementations that might be affected by the new Drop bound rg -l "impl.*Model.*ptr_from_keys" | while read -r file; do echo "=== $file ===" rg "impl.*Model.*ptr_from_keys" "$file" -A 5 doneLength of output: 51
Script:
#!/bin/bash # Try multiple search strategies echo "=== Searching for Model implementations ===" ast-grep --pattern 'impl $_ for $_ { $$$ }' echo "=== Searching for ptr_from_keys usages ===" rg "ptr_from_keys" -A 3 echo "=== Searching in test files ===" fd "test" --type f --exec rg "ptr_from_keys" {} -A 3 \;Length of output: 66095
Script:
#!/bin/bash # Search for Model implementations containing ptr_from_keys echo "=== Searching for Model implementations with ptr_from_keys ===" ast-grep --pattern 'impl $_ for $_ { $$$ fn ptr_from_keys$_($_) $_ $$$ }' echo "=== Searching for ptr_from_keys method calls ===" rg "ptr_from_keys" -B 2 -A 2Length of output: 2892
crates/dojo/core-cairo-test/src/tests/model/model.cairo (1)
198-206: Excellent test coverage for pointer creation methods, sensei!The test comprehensively verifies that all pointer creation methods produce equivalent results.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2940 +/- ##
==========================================
- Coverage 55.91% 55.90% -0.01%
==========================================
Files 446 446
Lines 57814 57814
==========================================
- Hits 32325 32321 -4
- Misses 25489 25493 +4 ☔ View full report in Codecov by Sentry. |
If yes, could you please update #2939? |
glihm
changed the title
…n methods
Description
added methods to make mutiple model ptrs and tests
Related issue
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
ModelPtrtypeTests
Improvements
Foostruct to support equality comparisonsModelImpltrait to includeDroptrait requirement