feat: Improve bindgen types #2773
Conversation
WalkthroughOhayo, sensei! This pull request introduces several updates primarily focused on dependency management in the Changes
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: 3
🧹 Outside diff range and nitpick comments (5)
crates/dojo/bindgen/src/plugins/typescript/generator/enum.rs (1)
Line range hint
138-138: Typo in Test Function NameOhayo sensei! There is a minor typo in the test function name
test_it_does_not_duplicates_enum. It should betest_it_does_not_duplicate_enumto reflect proper grammar.Apply this diff to fix the typo:
- fn test_it_does_not_duplicates_enum() { + fn test_it_does_not_duplicate_enum() {crates/dojo/bindgen/src/plugins/typescript/generator/interface.rs (1)
137-150: Enhance Test Coverage intest_check_importOhayo sensei! The
test_check_importfunction currently asserts only the length of the buffer. To ensure that the correct imports are being added or not added, consider enhancing the test to assert the actual contents of the buffer after each call tocheck_import. This will improve the test coverage and catch any potential issues with import management.Apply this diff to enhance the test:
fn test_check_import() { let mut buff = Buffer::new(); let writer = TsInterfaceGenerator; let token = create_test_struct_token(); writer.check_import(&token, &mut buff); assert_eq!(1, buff.len()); + assert!(buff[0].contains("import type { BigNumberish } from 'starknet';")); let option = create_option_token(); writer.check_import(&option, &mut buff); assert_eq!(1, buff.len()); // Shouldn't add duplicate imports + // Check that CairoOption import is added + assert!(buff[0].contains("CairoOption")); let custom_enum = create_custom_enum_token(); writer.check_import(&custom_enum, &mut buff); assert_eq!(1, buff.len()); // Shouldn't add duplicate imports + // Additional assertions can be added here if necessary }crates/dojo/bindgen/src/plugins/typescript/generator/schema.rs (1)
93-95: Consider Using a More Robust Method for Checking Namespace DefinitionOhayo sensei! In the
namespace_is_definedmethod, the current implementation checks for the namespace definition in the buffer using a string match. This approach might be fragile and could fail if the formatting changes. Consider using a more robust method, such as parsing the buffer into an abstract syntax tree (AST) or maintaining a separate data structure to track the defined namespaces.crates/dojo/bindgen/src/plugins/typescript/writer.rs (1)
54-56: Ohayo sensei! Remove commented debug print statements.Instead of commenting out debug statements, it's better to remove them entirely. If debugging is needed in the future, we can rely on proper logging mechanisms or version control history.
- // println!("Generating code for model {}", c.type_path); - // println!("{:#?}", c); - // println!("=====================");crates/dojo/bindgen/src/plugins/typescript/generator/function.rs (1)
91-96: Consider refactoring to improve maintainabilityThe type prefix logic could be enhanced for better maintainability:
- Extract magic strings as constants
- Simplify nested conditions using pattern matching
+ const CORE_PREFIX: &str = "core"; + const MODELS_PREFIX: &str = "models."; + const MODELS_INPUT_PREFIX: &str = "models.Input"; match &input.1 { Token::Composite(t) => match t.r#type { CompositeType::Enum => MODELS_PREFIX, CompositeType::Struct if !t.type_path.starts_with(CORE_PREFIX) => MODELS_INPUT_PREFIX, _ => "", }, _ => "", }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.toml(2 hunks)crates/dojo/bindgen/src/plugins/mod.rs(1 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/constants.rs(1 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/enum.rs(1 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/function.rs(1 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/interface.rs(3 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs(6 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/schema.rs(7 hunks)crates/dojo/bindgen/src/plugins/typescript/writer.rs(1 hunks)
🔇 Additional comments (4)
crates/dojo/bindgen/src/plugins/mod.rs (1)
77-84: Ohayo sensei! The new Buffer method looks good!
The insert_at_index implementation is clean and well-documented, following the existing codebase style.
Cargo.toml (2)
74-75: Ohayo sensei! Good move using tagged versions!
Moving from commit hashes to tagged versions (v0.4.10) for cainome and cainome-cairo-serde improves dependency stability and version tracking.
213-213: Verify compatibility with serde_with 3.11.0
The update from serde_with 3.9.0 to 3.11.0 might include breaking changes.
✅ Verification successful
Let me check the actual usage patterns in the codebase to ensure the changes in the changelog won't affect the current implementation.
Ohayo! The update to serde_with 3.11.0 is safe to proceed, sensei!
The changelog shows only additions and fixes between 3.9.0 and 3.11.0:
- Added support for hashbrown v0.15
- Added new line separators (UnixLineSeparator, DosLineSeparator)
- Fixed cfg_attr handling in serde_as macro
Our codebase only uses the basic #[serde_with::serde_as] attribute macro on structs in the genesis module, which is unaffected by these changes. The improvements to cfg_attr handling actually makes the macro more robust.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in serde_with changelog
# Get the changelog or release notes for serde_with
gh api repos/jonasbb/serde_with/releases | jq -r '.[] | select(.tag_name | contains("3.10.0","3.11.0")) | .body'
Length of output: 660
Script:
#!/bin/bash
# Search for serde_with usage patterns to check compatibility
ast-grep --pattern 'use serde_with::{$$$}'
# Also check for any direct attribute usage
rg '#\[serde_with' -A 2
Length of output: 1172
crates/dojo/bindgen/src/plugins/typescript/generator/function.rs (1)
91-96: Ohayo sensei! The type handling logic looks good! ✨
The separation of enum and struct type handling improves clarity and maintainability.
| if !buffer.has(CAIRO_ENUM_TYPE_IMPL) { | ||
| let pos = buffer.pos(SN_IMPORT_SEARCH).unwrap(); | ||
| buffer.insert_at_index(CAIRO_ENUM_TYPE_IMPL.to_owned(), pos + 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential Panic due to Unwrap on None
Ohayo sensei! In the check_import method, using buffer.pos(SN_IMPORT_SEARCH).unwrap() may cause a panic if SN_IMPORT_SEARCH is not found in the buffer. To prevent this, consider handling the Option returned by buffer.pos gracefully, perhaps using if let Some(pos) = buffer.pos(SN_IMPORT_SEARCH) or providing a default value.
Apply this diff to fix the potential panic:
- let pos = buffer.pos(SN_IMPORT_SEARCH).unwrap();
+ if let Some(pos) = buffer.pos(SN_IMPORT_SEARCH) {
+ buffer.insert_at_index(CAIRO_ENUM_TYPE_IMPL.to_owned(), pos + 1);
+ } else {
+ // Handle the case where SN_IMPORT_SEARCH is not found
+ buffer.insert(CAIRO_ENUM_TYPE_IMPL.to_owned());
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !buffer.has(CAIRO_ENUM_TYPE_IMPL) { | |
| let pos = buffer.pos(SN_IMPORT_SEARCH).unwrap(); | |
| buffer.insert_at_index(CAIRO_ENUM_TYPE_IMPL.to_owned(), pos + 1); | |
| } | |
| } | |
| if !buffer.has(CAIRO_ENUM_TYPE_IMPL) { | |
| if let Some(pos) = buffer.pos(SN_IMPORT_SEARCH) { | |
| buffer.insert_at_index(CAIRO_ENUM_TYPE_IMPL.to_owned(), pos + 1); | |
| } else { | |
| // Handle the case where SN_IMPORT_SEARCH is not found | |
| buffer.insert(CAIRO_ENUM_TYPE_IMPL.to_owned()); | |
| } | |
| } | |
| } |
| match value.inners[0].token.to_composite() { | ||
| Ok(c) => JsDefaultValue::from(c), | ||
| Err(_) => { | ||
| JsDefaultValue(format!("{}.{}", value.type_name(), value.inners[0].name)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential Index Out of Bounds Panic in JsDefaultValue::from
Ohayo sensei! In the JsDefaultValue::from implementation for enums, accessing value.inners[0] without checking if inners is non-empty may cause an index out of bounds panic if inners is empty. To prevent this, consider adding a check to ensure value.inners is not empty before accessing the first element.
Apply this diff to fix the potential panic:
match value.r#type {
CompositeType::Enum => {
- match value.inners[0].token.to_composite() {
+ if let Some(first_inner) = value.inners.first() {
+ match first_inner.token.to_composite() {
Ok(c) => JsDefaultValue::from(c),
Err(_) => {
- JsDefaultValue(format!("{}.{}", value.type_name(), value.inners[0].name))
+ JsDefaultValue(format!("{}.{}", value.type_name(), first_inner.name))
}
}
+ } else {
+ // Handle the case where inners is empty
+ JsDefaultValue("undefined".to_string())
+ }
}
// other cases...
}Committable suggestion skipped: line range outside the PR's diff.
| @@ -14,3 +14,20 @@ pub const CAIRO_BOOL: &str = "bool"; | |||
| pub const JS_BOOLEAN: &str = "boolean"; | |||
| pub const JS_STRING: &str = "string"; | |||
| pub const JS_BIGNUMBERISH: &str = "BigNumberish"; | |||
|
|
|||
| pub(crate) const BIGNUMNERISH_IMPORT: &str = "import type { BigNumberish } from 'starknet';\n"; | |||
There was a problem hiding this comment.
Typo in Constant Name BIGNUMNERISH_IMPORT
Ohayo sensei! There is a typo in the constant name BIGNUMNERISH_IMPORT. It should be BIGNUMBERISH_IMPORT to match the correct spelling of BigNumberish.
Apply this diff to fix the typo:
- pub(crate) const BIGNUMNERISH_IMPORT: &str = "import type { BigNumberish } from 'starknet';\n";
+ pub(crate) const BIGNUMBERISH_IMPORT: &str = "import type { BigNumberish } from 'starknet';\n";Also, update all references to BIGNUMNERISH_IMPORT throughout the codebase to match the corrected name.
Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2773 +/- ##
==========================================
- Coverage 56.24% 55.80% -0.45%
==========================================
Files 415 444 +29
Lines 53241 57319 +4078
==========================================
+ Hits 29948 31989 +2041
- Misses 23293 25330 +2037 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I don't have much ts perspective to evaluate the generated code. Will iterate with the contributors currently working around that. Thanks for the unlock @MartianGreed!
We should think of a way to have functional test with the bindgen. That would be great to ensure we don't break and the generated code being actually functional in the way it is intended to be. 👍
Description
Long awaited PR is finally there. Mainly solves #2715
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
Bufferstruct.Bug Fixes
Tests