fix: function mappings + object naming #2651
Conversation
WalkthroughOhayo! This pull request introduces significant modifications to the Changes
Assessment against linked issues
Possibly related PRs
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
🧹 Outside diff range and nitpick comments (1)
crates/dojo/bindgen/src/plugins/typescript/generator/function.rs (1)
Line range hint
350-417: The test coverage looks good, but could be even better, sensei!While the current tests cover the basic duplicate prevention scenarios, consider adding these test cases:
- Test with mixed snake_case and camelCase functions in the same contract
- Test with namespace variations
Here's a suggested test addition:
#[test] fn test_mixed_case_functions_with_namespace() { let generator = TsFunctionGenerator {}; let mut buff = Buffer::new(); // Test both cases with different namespaces let snake_case_fn = create_change_theme_function(); let camel_case_fn = create_change_theme_function_camelized(); let contract1 = DojoContract { tag: "namespace1-actions".to_owned(), tokens: TokenizedAbi::default(), systems: vec![], }; let contract2 = DojoContract { tag: "namespace2-actions".to_owned(), tokens: TokenizedAbi::default(), systems: vec![], }; let _ = generator.generate(&contract1, &snake_case_fn, &mut buff); let _ = generator.generate(&contract1, &camel_case_fn, &mut buff); let _ = generator.generate(&contract2, &snake_case_fn, &mut buff); // Assert expected buffer state assert_eq!(6, buff.len()); // Should not duplicate functions assert!(buff.has("namespace1")); // Should include first namespace assert!(buff.has("namespace2")); // Should include second namespace }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/dojo/bindgen/src/plugins/typescript/generator/function.rs(10 hunks)
🔇 Additional comments (4)
crates/dojo/bindgen/src/plugins/typescript/generator/function.rs (4)
30-35: Ohayo! The namespace parameter addition looks good, sensei!
This change directly addresses the namespace issue mentioned in #2628, ensuring proper namespace inclusion in provider.execute calls.
67-70: Nice type mapping improvements, sensei!
The additional checks for enum and non-core struct types ensure proper namespace prefixing, which helps prevent type resolution issues in the generated TypeScript code.
182-196: Excellent duplicate prevention logic, sensei!
The added checks effectively prevent function duplication when both snake_case and camelCase versions exist, addressing a key issue from #2628.
40-46: The execute call parameters have been properly aligned, sensei!
The changes address multiple issues from #2628:
- Using
snAccountprevents duplicate identifier errors - Added namespace parameter ensures proper execution context
- Fixed casing of
entrypointfor consistency
Let's verify the entrypoint casing consistency across the codebase:
✅ Verification successful
All clear on the entrypoint casing, sensei!
The codebase consistently uses lowercase entrypoint in TypeScript-related code and maintains appropriate casing conventions in other contexts. The changes in the review are correct and align with the TypeScript conventions while not conflicting with other usage patterns:
- TypeScript/JavaScript files use lowercase
entrypoint - Rust code appropriately uses snake_case
entry_pointor PascalCaseEntryPointbased on context (types, enums, etc.) - Documentation and comments maintain consistent casing within their respective domains
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining instances of 'entryPoint' that might need updating
rg -i "entryPoint" --type rust
Length of output: 10513
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2651 +/- ##
==========================================
+ Coverage 56.83% 57.43% +0.60%
==========================================
Files 397 400 +3
Lines 49535 49963 +428
==========================================
+ Hits 28151 28698 +547
+ Misses 21384 21265 -119 ☔ View full report in Codecov by Sentry. |
Description
Closes #2628
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh) cc @glihm, I swear this is truescripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
Bug Fixes
Tests