-
Notifications
You must be signed in to change notification settings - Fork 179
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
feat: introspection for bytes31 #2661
Conversation
WalkthroughOhayo, sensei! This pull request introduces a new entry in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Function
participant Output
User->>Function: Request primitive type introspection
Function->>Output: Return mapping of primitive types
Output-->>User: Includes "bytes31" with (1, vec![248])
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
|
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/dojo/lang/src/derive_macros/introspect/utils.rs (1)
Line range hint
9-19
: Consider documenting the introspection value selection.The implementation uses TypeIntrospection(1, vec![248]) for bytes31, similar to how felt252 uses vec![251] and other numeric types use their bit sizes. It would be helpful to document why 248 was chosen for bytes31.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/dojo/lang/src/derive_macros/introspect/utils.rs
(1 hunks)
🔇 Additional comments (1)
crates/dojo/lang/src/derive_macros/introspect/utils.rs (1)
9-9
: Ohayo sensei! The bytes31 introspection implementation looks solid!
The implementation follows the established pattern for primitive types, using a TypeIntrospection with size 1 and a descriptor value of 248. This aligns well with other fixed-size types in the collection.
Let's verify the consistency of this implementation:
✅ Verification successful
Ohayo sensei! The bytes31 introspection implementation is perfectly aligned with the codebase patterns!
The implementation follows the established pattern where:
- All primitive types use TypeIntrospection with size 1
- The descriptor value 248 for bytes31 fits well in the sequence with other types
- The placement in the HashMap is logical, grouped with other primitive types
- The type is properly handled in bindgen plugins (typescript, unity, recs)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that bytes31 isn't already handled elsewhere and check for related tests
# Check for any other bytes31 type handling
rg -i "bytes31" --type rust
# Look for related test cases
rg -i "test.*bytes31" --type rust
Length of output: 471
Script:
#!/bin/bash
# Let's verify the implementation pattern by checking other similar types
# Check the context around bytes31 implementation
rg -B 5 -A 5 "bytes31" crates/dojo/lang/src/derive_macros/introspect/utils.rs
# Look for other byte-related type implementations
ast-grep --pattern 'TypeIntrospection($_, vec![$_])'
Length of output: 1980
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.
That's a good catch, thank you @shramee for PRing it!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2661 +/- ##
==========================================
- Coverage 57.49% 57.49% -0.01%
==========================================
Files 400 400
Lines 50276 50277 +1
==========================================
Hits 28905 28905
- Misses 21371 21372 +1 ☔ View full report in Codecov by Sentry. |
Description
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
"bytes31"
for enhanced type mapping.Bug Fixes
Documentation