feat: update bindgen for dojo.js sdk #2501
Conversation
WalkthroughOhayo, sensei! This pull request introduces a new dependency on the Changes
Possibly related PRs
Suggested labels
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: 25
🧹 Outside diff range and nitpick comments (11)
crates/dojo-bindgen/src/plugins/typescript/generator/enum.rs (2)
7-29: Excellent implementation, sensei! Just a small suggestion.The
generatemethod looks well-structured and handles edge cases appropriately. The use offormat!macro makes the code readable and maintainable. Great job on including a comment with the enum's path in the generated TypeScript code!Consider adding a newline after the closing brace of the enum definition to improve readability of the generated code. You can modify line 17 like this:
-}} +}} +
31-110: Ohayo, sensei! Impressive test coverage, but let's make it even better!The test module is well-structured with three unit tests covering important scenarios. The helper function
create_available_theme_enum_tokenis a great addition for code reusability.To improve readability of the third test, consider using a multi-line string for the expected output. You can modify lines 78-79 like this:
- assert_eq!(result, "// Type definition for `core::test::AvailableTheme` enum\nexport enum AvailableTheme {\n\tLight,\n\tDark,\n\tDojo,\n}\n"); + assert_eq!(result, r#"// Type definition for `core::test::AvailableTheme` enum +export enum AvailableTheme { + Light, + Dark, + Dojo, +} +"#);This change will make it easier to visually inspect the expected output and maintain the test in the future.
crates/dojo-bindgen/src/plugins/recs/tests.rs (1)
14-14: Ohayo, sensei! LGTM with a small suggestion.The plugin instantiation has been correctly updated to use
TypescriptRecsPlugin::new(). However, consider updating the test function name to reflect the new plugin:-async fn test_typescript_plugin_generate_code() { +async fn test_typescript_recs_plugin_generate_code() {This change would improve consistency and clarity in the test suite.
crates/dojo-bindgen/src/lib.rs (1)
93-93: Ohayo, sensei! The new plugin case is well-integrated!The addition of
BuiltinPlugins::Recscase in thegeneratemethod is correct and follows the existing pattern. Great job on maintaining consistency!As a small suggestion for improved readability:
Consider grouping similar plugins together. For instance, you could place
Recsnext toTypeScriptV2since they both seem to be TypeScript-related. This might make it easier to manage as the number of plugins grows.match plugin { BuiltinPlugins::Typescript => Box::new(TypescriptPlugin::new()), - BuiltinPlugins::Unity => Box::new(UnityPlugin::new()), BuiltinPlugins::TypeScriptV2 => Box::new(TypeScriptV2Plugin::new()), BuiltinPlugins::Recs => Box::new(TypescriptRecsPlugin::new()), + BuiltinPlugins::Unity => Box::new(UnityPlugin::new()), }crates/dojo-bindgen/src/plugins/typescript/mod.rs (1)
28-40: Ohayo, sensei! Consider making file names configurableCurrently, the file names
"models.gen.ts"and"contracts.gen.ts"are hardcoded. Making these file names configurable would enhance flexibility and allow users to specify custom output paths if needed.Consider applying this change:
writers: vec![ Box::new(TsFileWriter::new( - "models.gen.ts", + configurable_models_file_name, vec![ Box::new(TsInterfaceGenerator {}), Box::new(TsEnumGenerator {}), Box::new(TsSchemaGenerator {}), ], )), Box::new(TsFileContractWriter::new( - "contracts.gen.ts", + configurable_contracts_file_name, vec![Box::new(TsFunctionGenerator {})], )), ],This change would require passing the file names as parameters to the
newmethod.crates/dojo-bindgen/src/plugins/mod.rs (1)
11-11: Consider adding documentation for the newrecsmoduleSensei, adding a brief comment explaining the purpose of the
recsmodule abovepub mod recs;would enhance code readability for other developers.crates/dojo-bindgen/src/plugins/typescript/generator/mod.rs (1)
3-3: Consider renamingr#enummodule to avoid reserved keyword usageOhayo, sensei! Using
r#enumas a module name employs a raw identifier to bypass Rust's reserved keywords. While this is syntactically correct, it might reduce code readability and could confuse other developers.Consider renaming the module to improve clarity:
-pub(crate) mod r#enum; +pub(crate) mod enums;crates/dojo-bindgen/src/plugins/typescript/writer.rs (1)
74-77: Ohayo, sensei! Ensure consistent return types forget_pathmethodsThe
get_pathmethod inTsFileWriterreturns&'static str, while inTsFileContractWriter, it returns&str. For consistency and flexibility, consider using&strin both implementations unless a'staticlifetime is specifically required.Adjust
TsFileWriter'sget_pathmethod:- fn get_path(&self) -> &'static str { + fn get_path(&self) -> &str {This change aligns the return types across both structs.
Also applies to: 137-139
crates/dojo-bindgen/src/plugins/typescript/generator/function.rs (1)
118-267: Expand unit tests to cover more scenariosOhayo, sensei! The current unit tests cover basic functionality. Consider adding tests for edge cases, such as:
- Functions with no inputs or outputs.
- Functions with complex input types (e.g., arrays, custom types).
- Situations where the buffer already contains multiple entries.
This will help ensure the generator handles all possible use cases correctly.
crates/dojo-bindgen/src/plugins/typescript/generator/schema.rs (2)
10-12: Correct typos in documentation commentsOhayo, sensei! There are typographical errors in the comments on lines 10-12. For example, "fieleds" should be "fields." Also, consider improving the clarity and grammar of the comments.
Apply this diff to correct the typos and enhance the documentation:
-/// first we need to generate interface of schema which will be used in dojo.js sdk to fully type -/// sdk -/// then we need to build the schema const which contains default values for all fieleds +/// First, we need to generate the interface of the schema, which will be used in the Dojo.js SDK to fully type the SDK. +/// Then, we need to build the schema constant, which contains default values for all fields.
137-140: Correct typo in test module commentsOhayo, sensei! There's a typographical error in the comment on line 137. "Supporsed" should be "supposed."
Apply this diff to correct the typo:
-/// Those tests may not test the returned value because it is supporsed to be called sequentially +/// These tests may not test the returned value because it is supposed to be called sequentially
📜 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 (12)
- crates/dojo-bindgen/Cargo.toml (1 hunks)
- crates/dojo-bindgen/src/lib.rs (2 hunks)
- crates/dojo-bindgen/src/plugins/mod.rs (3 hunks)
- crates/dojo-bindgen/src/plugins/recs/mod.rs (1 hunks)
- crates/dojo-bindgen/src/plugins/recs/tests.rs (4 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 (1 hunks)
- crates/dojo-bindgen/src/plugins/typescript/generator/mod.rs (1 hunks)
- crates/dojo-bindgen/src/plugins/typescript/generator/schema.rs (1 hunks)
- crates/dojo-bindgen/src/plugins/typescript/mod.rs (1 hunks)
- crates/dojo-bindgen/src/plugins/typescript/writer.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (20)
crates/dojo-bindgen/Cargo.toml (1)
14-14: Ohayo, sensei! LGTM: New logging dependency added.The addition of the 'log' crate looks good. This will enable proper logging functionality in the project, which is essential for debugging and monitoring. The use of '.workspace = true' ensures consistent versioning across the workspace.
crates/dojo-bindgen/src/plugins/typescript/generator/enum.rs (1)
1-5: Ohayo, sensei! The imports and struct definition look good!The necessary types are imported, and the
TsEnumGeneratorstruct is defined concisely. Great job on keeping it simple and focused!crates/dojo-bindgen/src/plugins/recs/tests.rs (7)
9-9: Ohayo, sensei! LGTM: Import statement updated correctly.The import statement has been properly updated to use
TypescriptRecsPlugin, which aligns with the transition fromTypescriptPlugintoTypescriptRecsPluginthroughout the file.
41-41: Ohayo, sensei! LGTM: Static method calls updated correctly.The static method calls have been properly updated to use
TypescriptRecsPlugin::map_type(). The expected results remain unchanged, which suggests that the functionality of the new plugin is consistent with the previous implementation.Also applies to: 45-45
50-51: Ohayo, sensei! LGTM: Contract name formatting tests updated correctly.The static method calls in
test_formatted_contract_namehave been properly updated to useTypescriptRecsPlugin::formatted_contract_name(). The expected results are unchanged, confirming that the new plugin maintains the same contract name formatting logic.
56-60: Ohayo, sensei! LGTM: Namespace extraction tests updated correctly.The static method calls in
test_get_namespace_from_taghave been properly updated to useTypescriptRecsPlugin::get_namespace_from_tag(). The expected results are unchanged, confirming that the new plugin maintains the same namespace extraction logic.
Line range hint
1-185: Ohayo, sensei! Overall LGTM with a final suggestion.The changes in this file consistently update the usage from
TypescriptPlugintoTypescriptRecsPlugin. This appears to be part of a larger refactoring to introduce the newTypescriptRecsPlugin. The tests have been updated to use the new plugin while maintaining the same expected outputs, suggesting that the new plugin is intended to preserve the functionality of the old one.To ensure that all tests pass with the new implementation, it would be wise to run the entire test suite for this module.
#!/bin/bash # Description: Run all tests in the recs module to ensure compatibility with TypescriptRecsPlugin # Test: Run all tests in the recs module cargo test --package dojo-bindgen --lib -- plugins::recs::testsAdditionally, consider adding new tests specifically for any new functionality introduced by
TypescriptRecsPluginthat wasn't present in the originalTypescriptPlugin.
153-153: Ohayo, sensei! LGTM with a small suggestion.The static method call has been correctly updated to use
TypescriptRecsPlugin::format_model()for enum models. As with the previous test, it's crucial to verify that the expected output string still matches the new plugin's output for enum models. Consider running this test to ensure the expected result hasn't changed with the new implementation.#!/bin/bash # Description: Verify that the test_format_enum_model function passes with the new TypescriptRecsPlugin # Test: Run the specific test and check its output cargo test --package dojo-bindgen --lib -- plugins::recs::tests::test_format_enum_model --exact --nocapture
106-106: Ohayo, sensei! LGTM with a small suggestion.The static method call has been correctly updated to use
TypescriptRecsPlugin::format_model(). However, it's important to verify that the expected output string still matches the new plugin's output. Consider running this test to ensure the expected result hasn't changed with the new implementation.crates/dojo-bindgen/src/lib.rs (2)
14-14: Ohayo, sensei! New plugin import looks good!The import for
TypescriptRecsPluginis correctly placed and follows the existing naming convention for plugin imports.
Line range hint
1-293: Ohayo, sensei! Don't forget about testing!The changes for integrating the new
TypescriptRecsPluginlook good overall. However, to ensure the reliability of this new feature:Consider adding tests for the
TypescriptRecsPluginin thetestsmodule at the end of this file. This will help verify that the new plugin is correctly initialized and used within thePluginManager::generatemethod.Here's a shell script to check if there are any existing tests for the new plugin:
If this script doesn't find any relevant tests, it would be beneficial to add some.
crates/dojo-bindgen/src/plugins/typescript/mod.rs (1)
20-22: Ohayo, sensei! Great use of modular writers inTypescriptPluginThe addition of the
writersfield enhances the modularity and extensibility of the plugin. This approach allows for easier integration of new writers in the future.crates/dojo-bindgen/src/plugins/mod.rs (4)
30-30: Verify string representation consistency forBuiltinPlugins::RecsIn the
fmt::Displayimplementation,BuiltinPlugins::Recsis represented as"recs". Please confirm that this string aligns with expected usage elsewhere in the codebase to prevent any discrepancies.#!/bin/bash # Description: Check where `BuiltinPlugins` are converted to strings and ensure `"recs"` is used consistently. # Expected: Instances where `BuiltinPlugins::Recs`'s string representation is utilized. rg --type rust 'BuiltinPlugins::(Recs|Typescript|Unity|TypeScriptV2)' crates/dojo-bindgen/src/
36-36:⚠️ Potential issueOhayo, sensei! Ensure all
BuiltinPluginimplementations areSyncAdding
Syncto theBuiltinPlugintrait implies that all implementations must be thread-safe. Please verify that existing implementations ofBuiltinPluginsatisfy theSyncrequirement to avoid concurrency issues.#!/bin/bash # Description: Identify implementations of `BuiltinPlugin` and check for `Sync` compliance. # Expected: Implementations where all fields are `Sync`. rg --type rust 'impl.*BuiltinPlugin' crates/dojo-bindgen/src/ # For each implementation found, ensure that their members implement `Sync`.
9-9: EnsureDojoContractimport is necessaryThe import statement
use crate::{DojoContract, DojoData};includesDojoContract. Please verify thatDojoContractis utilized in this module. If it's not used, removing it can help maintain code clarity.#!/bin/bash # Description: Check if `DojoContract` is used in this file. # Expected: Lines where `DojoContract` is referenced. rg --type rust 'DojoContract' crates/dojo-bindgen/src/plugins/mod.rs
6-6: Ohayo, sensei! Verify the necessity of importingCompositeandFunctionThe added imports of
CompositeandFunctionfromcainome::parser::tokensshould be utilized within this module. Please confirm that these imports are required. If they are not used, consider removing them to keep the codebase clean.crates/dojo-bindgen/src/plugins/typescript/generator/mod.rs (2)
14-14: Verify ifByteArrayshould map tostringOhayo, sensei!
ByteArrayis currently mapped to a JavaScriptstring. IfByteArrayrepresents binary data, it might be more appropriate to map it toUint8ArrayorBufferin JavaScript for accurate representation.Would you like to confirm the correct mapping for
ByteArray?
21-21: Ensure unmapped types are correctly handled inJsTypeOhayo, sensei! The default case in
JsTypesimply uses the original type name. This could result in JavaScript types that don't exist, potentially causing runtime errors.Consider handling unmapped types explicitly or providing a fallback type like
any.crates/dojo-bindgen/src/plugins/typescript/generator/interface.rs (1)
18-18: Question: Is 'fieldOrder' necessary in the generated interface?Ohayo, sensei! The
fieldOrderproperty is included in every generated TypeScript interface. If this property isn't essential for your interfaces, you might consider removing it to simplify the generated code.You can run the following script to check if
fieldOrderis used elsewhere in the codebase:crates/dojo-bindgen/src/plugins/recs/mod.rs (1)
1-618: Excellent work on modularizing thebindgencomponentOhayo sensei! The new
TypescriptRecsPluginimplementation significantly enhances the modularity and extensibility of thebindgencomponent. The code is well-structured, and the separation of concerns in functions likehandle_modelandhandle_contractsimproves maintainability.
| .map(|writer| match writer.write(writer.get_path(), data) { | ||
| Ok(c) => c, | ||
| Err(e) => { | ||
| log::error!("Failed to generate code for typescript plugin: {e}"); | ||
| ("".into(), Vec::new()) | ||
| } | ||
| }) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
Ohayo, sensei! Handle errors more robustly in generate_code
When an error occurs during code generation, the current implementation logs the error but still returns an entry with an empty path and code. This may lead to issues when processing the results, such as attempting to write to an invalid path.
Consider modifying the error handling to skip over writers that fail, preventing entries with empty paths from being inserted into the output map. Apply this diff to improve error handling:
.collect::<Vec<_>>();
+ let code = code.into_iter().filter(|(path, code)| !path.is_empty() && !code.is_empty()).collect::<Vec<_>>();Alternatively, you can adjust the iterator to skip errors:
- .map(|writer| match writer.write(writer.get_path(), data) {
+ .filter_map(|writer| match writer.write(writer.get_path(), data) {
Ok(c) => Some(c),
Err(e) => {
log::error!("Failed to generate code for typescript plugin: {e}");
- ("".into(), Vec::new())
+ None
}
})This change ensures that only successful code generations are included in the output.
Committable suggestion was skipped due to low confidence.
| @@ -16,6 +18,7 @@ pub enum BuiltinPlugins { | |||
| Typescript, | |||
| Unity, | |||
| TypeScriptV2, | |||
| Recs, | |||
There was a problem hiding this comment.
Update BuiltinPlugins documentation to include Recs
With the addition of the Recs variant to the BuiltinPlugins enum, please ensure that any associated documentation or comments are updated accordingly.
|
|
||
| pub trait BindgenWriter: Sync { | ||
| /// Writes the generated code to the specified path. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `code` - The generated code. | ||
| fn write(&self, path: &str, data: &DojoData) -> BindgenResult<(PathBuf, Vec<u8>)>; | ||
| fn get_path(&self) -> &str; | ||
| } | ||
|
|
||
| pub trait BindgenModelGenerator: Sync { | ||
| /// Generates code by executing the plugin. | ||
| /// The generated code is written to the specified path. | ||
| /// This will write file sequentially (for now) so we need one generator per part of the file. | ||
| /// (header, type definitions, interfaces, functions and so on) | ||
| /// TODO: add &mut ref to what's currently generated to place specific code at specific places. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// | ||
| fn generate(&self, token: &Composite, buffer: &mut Vec<String>) -> BindgenResult<String>; | ||
| } | ||
|
|
||
| pub trait BindgenContractGenerator: Sync { | ||
| fn generate( | ||
| &self, | ||
| contract: &DojoContract, | ||
| token: &Function, | ||
| buffer: &mut Vec<String>, | ||
| ) -> BindgenResult<String>; | ||
| } |
There was a problem hiding this comment.
Document the new code generation traits thoroughly
The introduction of BindgenWriter, BindgenModelGenerator, and BindgenContractGenerator traits is a significant enhancement. To assist other developers:
- Provide clear documentation for each trait and their methods.
- Explain the intended use cases and any important implementation details.
- Ensure all parameters and return types are well-described.
| match value { | ||
| "felt252" => JsDefaultValue("0".to_string()), | ||
| "ContractAddress" => JsDefaultValue("\"\"".to_string()), | ||
| "ByteArray" => JsDefaultValue("\"\"".to_string()), |
There was a problem hiding this comment.
Set appropriate default value for ByteArray
Ohayo, sensei! The default value for ByteArray is currently "\"\"", which represents an empty string. If ByteArray should represent binary data, consider using an empty array [] or a typed array like new Uint8Array().
Update the default value to better reflect an empty byte array:
"ByteArray" => JsDefaultValue("[]".to_string()),📝 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.
| "ByteArray" => JsDefaultValue("\"\"".to_string()), | |
| "ByteArray" => JsDefaultValue("[]".to_string()), |
| _ => JsDefaultValue(value.to_string()), | ||
| } |
There was a problem hiding this comment.
Handle default values for unmapped types carefully
Ohayo, sensei! Using value.to_string() in the default case may not yield valid JavaScript default values, especially for complex or custom types.
Consider setting a generic default value like null or {}:
_ => JsDefaultValue("null".to_string()),Or handle specific cases as needed.
Committable suggestion was skipped due to low confidence.
| // filter out common types | ||
| // TODO: Make cleaner | ||
| if token.type_path == "core::option::Option::<core::integer::u32>" | ||
| || token.type_path == "core::option::Option::<core::integer::u8>" | ||
| || token.type_path == "core::option::Option::<core::integer::u16>" | ||
| || token.type_path == "core::option::Option::<core::integer::u64>" | ||
| || token.type_path == "core::option::Option::<core::integer::u128>" | ||
| || token.type_path == "core::option::Option::<core::integer::u256>" | ||
| { | ||
| return String::new(); // Return an empty string for these enums | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor hardcoded enum filters for maintainability
Ohayo sensei! The code in format_enum filters out certain common types by hardcoding their paths (lines 145~ to 150~). To enhance maintainability and scalability, consider replacing this approach with a constant list or configuration to manage these types more efficiently.
You can define a constant array to store the filtered enum paths:
const FILTERED_ENUMS: &[&str] = &[
"core::option::Option::<core::integer::u32>",
"core::option::Option::<core::integer::u8>",
"core::option::Option::<core::integer::u16>",
"core::option::Option::<core::integer::u64>",
"core::option::Option::<core::integer::u128>",
"core::option::Option::<core::integer::u256>",
];
if FILTERED_ENUMS.contains(&token.type_path.as_str()) {
return String::new(); // Return an empty string for these enums
}| match token { | ||
| Token::CoreBasic(_) => TypescriptRecsPlugin::map_type(token) | ||
| .replace("RecsType.", "").replace("Array", "[]") | ||
| // types should be lowercased | ||
| .to_lowercase(), | ||
| Token::Composite(t) => format!("models.{}", t.type_name()), | ||
| Token::Array(_) => TypescriptRecsPlugin::map_type(token), | ||
| _ => panic!("Unsupported token type: {:?}", token), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid code duplication by reusing the existing map_type function
Ohayo sensei! The map_type function defined within format_system (lines 354~ to 363~) duplicates functionality of the main map_type method. To adhere to the DRY (Don't Repeat Yourself) principle, consider reusing the existing map_type function or refactoring the code to eliminate redundancy.
You can modify the inner map_type to call the existing map_type and adjust types accordingly:
fn map_type(token: &Token) -> String {
TypescriptRecsPlugin::map_type(token)
.replace("RecsType.", "")
.replace("Array", "[]")
.to_lowercase()
}This way, you utilize the existing logic and maintain consistency across your type mappings.
| custom_types.push(format!("\"{}\"", field.token.type_name())); | ||
| format!("{}: {}Definition,", field.name, mapped) | ||
| } | ||
| _ if mapped == field.token.type_name() => { | ||
| custom_types.push(format!("\"{}\"", field.token.type_name())); | ||
| format!("{}: {}Definition,", field.name, mapped) | ||
| } | ||
| _ => { | ||
| types.push(format!("\"{}\"", field.token.type_name())); | ||
| format!("{}: {},", field.name, mapped) | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify field handling logic in format_model
Ohayo sensei! In format_model, the match arms for field handling (lines 217~ to 228~) contain some duplicated code when adding to custom_types and formatting field_str. Simplifying this logic will enhance readability and maintainability.
Consider refactoring the match statement to reduce duplication:
let field_str = match field.token {
Token::Composite(ref c) if c.r#type == CompositeType::Enum => {
types.push(format!("\"{}\"", field.token.type_name()));
format!("{}: RecsType.String,", field.name)
}
Token::Composite(_) | _ if mapped == field.token.type_name() => {
custom_types.push(format!("\"{}\"", field.token.type_name()));
format!("{}: {}Definition,", field.name, mapped)
}
_ => {
types.push(format!("\"{}\"", field.token.type_name()));
format!("{}: {},", field.name, mapped)
}
};This refactoring merges common actions and reduces the number of match arms.
| let mut sorted_structs = tokens.structs.clone(); | ||
| sorted_structs.sort_by(compare_tokens_by_type_name); | ||
|
|
||
| let mut sorted_enums = tokens.enums.clone(); | ||
| sorted_enums.sort_by(compare_tokens_by_type_name); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply the DRY principle to sorting logic
Ohayo sensei! In handle_model, both sorted_structs and sorted_enums are sorted using the same comparator (lines 281~ and 284~). To follow the DRY principle, consider abstracting the sorting into a helper function to avoid repetition.
You can create a helper function:
fn sort_composites(composites: &mut Vec<Composite>) {
composites.sort_by(compare_tokens_by_type_name);
}
// Then use it:
sort_composites(&mut sorted_structs);
sort_composites(&mut sorted_enums);| fn map_type(token: &Token) -> String { | ||
| match token.type_name().as_str() { | ||
| "bool" => "RecsType.Boolean".to_string(), | ||
| "i8" => "RecsType.Number".to_string(), | ||
| "i16" => "RecsType.Number".to_string(), | ||
| "i32" => "RecsType.Number".to_string(), | ||
| "i64" => "RecsType.Number".to_string(), | ||
| "i128" => "RecsType.BigInt".to_string(), | ||
| "u8" => "RecsType.Number".to_string(), | ||
| "u16" => "RecsType.Number".to_string(), | ||
| "u32" => "RecsType.Number".to_string(), | ||
| "u64" => "RecsType.Number".to_string(), | ||
| "u128" => "RecsType.BigInt".to_string(), | ||
| "u256" => "RecsType.BigInt".to_string(), | ||
| "usize" => "RecsType.Number".to_string(), | ||
| "felt252" => "RecsType.BigInt".to_string(), | ||
| "bytes31" => "RecsType.String".to_string(), | ||
| "ClassHash" => "RecsType.BigInt".to_string(), | ||
| "ContractAddress" => "RecsType.BigInt".to_string(), | ||
| "ByteArray" => "RecsType.String".to_string(), | ||
| "array" => { | ||
| if let Token::Array(array) = token { | ||
| let mut mapped = TypescriptRecsPlugin::map_type(&array.inner); | ||
| if mapped == array.inner.type_name() { | ||
| mapped = "RecsType.String".to_string(); | ||
| } | ||
|
|
||
| format!("{}Array", mapped) | ||
| } else { | ||
| panic!("Invalid array token: {:?}", token); | ||
| } | ||
| } | ||
| "generic_arg" => { | ||
| if let Token::GenericArg(g) = &token { | ||
| g.clone() | ||
| } else { | ||
| panic!("Invalid generic arg token: {:?}", token); | ||
| } | ||
| } | ||
|
|
||
| // we consider tuples as essentially objects | ||
| _ => { | ||
| let mut type_name = token.type_name().to_string(); | ||
|
|
||
| if let Token::Composite(composite) = token { | ||
| if !composite.generic_args.is_empty() { | ||
| type_name += &format!( | ||
| "<{}>", | ||
| composite | ||
| .generic_args | ||
| .iter() | ||
| .map(|(_, t)| TypescriptRecsPlugin::map_type(t)) | ||
| .collect::<Vec<_>>() | ||
| .join(", ") | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| type_name | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider replacing panics with proper error handling
Ohayo sensei! In the map_type function, there are instances where panic! is used (lines 52~ and 59~) to handle unexpected tokens. Using panic! can cause the program to crash unexpectedly. It's advisable to handle these cases gracefully by returning a Result or an Option, allowing the caller to manage the error appropriately.
Apply this diff to refactor the code:
- else {
- panic!("Invalid array token: {:?}", token);
- }
+ else {
+ return Err(format!("Invalid array token: {:?}", token));
+ }
- else {
- panic!("Invalid generic arg token: {:?}", token);
- }
+ else {
+ return Err(format!("Invalid generic arg token: {:?}", token));
+ }This change will require updating the function signature of map_type to return a Result<String, String> and handling the errors in the calling functions.
Committable suggestion was skipped due to low confidence.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2501 +/- ##
==========================================
+ Coverage 69.51% 69.79% +0.27%
==========================================
Files 388 396 +8
Lines 50076 51160 +1084
==========================================
+ Hits 34811 35707 +896
- Misses 15265 15453 +188 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
crates/dojo-bindgen/src/plugins/typescript/generator/interface.rs (1)
9-32: Ohayo, sensei! The implementation looks solid, with a small suggestion.The
generatemethod is well-implemented, handling different cases appropriately. It correctly formats the TypeScript interface based on the token's contents.For improved readability, consider using a raw string literal for the multi-line format string:
Ok(format!( r#"// Type definition for `{path}` struct export interface {name} {{ fieldOrder: string[]; {fields} }} "#, path = token.type_path, name = token.type_name(), fields = token .inners .iter() .map(|inner| format!(" {}: {};", inner.name, JsType::from(&inner.token))) .collect::<Vec<String>>() .join("\n") ))This change would make the template easier to maintain and align with the actual output.
crates/dojo-bindgen/src/plugins/typescript/generator/enum.rs (3)
7-35: Sugoi implementation of thegeneratemethod, sensei!The
generatemethod is well-structured and handles the necessary cases. However, consider the following suggestions:
- Add a comment explaining the purpose of the
bufferparameter and how it's used to avoid duplicates.- Consider extracting the enum generation logic into a separate private method for better readability and testability.
- Use
constfor the generated string template to improve performance.Here's a suggested refactoring:
impl BindgenModelGenerator for TsEnumGenerator { fn generate(&self, token: &Composite, buffer: &mut Vec<String>) -> BindgenResult<String> { if token.r#type != CompositeType::Enum || token.inners.is_empty() { return Ok(String::new()); } let gen = self.generate_enum_string(token); if buffer.iter().any(|b| b.contains(&gen)) { return Ok(String::new()); } Ok(gen) } } impl TsEnumGenerator { const ENUM_TEMPLATE: &'static str = "// Type definition for `{path}` enum export enum {name} {{ {variants} }} "; fn generate_enum_string(&self, token: &Composite) -> String { format!( Self::ENUM_TEMPLATE, path = token.type_path, name = token.type_name(), variants = token .inners .iter() .map(|inner| format!("\t{},", inner.name)) .collect::<Vec<String>>() .join("\n") ) } }
37-97: Dojo-level test coverage, sensei! But let's level up even more!The test cases cover the main scenarios well. However, consider the following suggestions to enhance the test suite:
- Add a test case for an enum with a single variant to ensure edge cases are covered.
- Consider parameterized tests to reduce code duplication and test more variants easily.
- Add a test case to verify the correct handling of enum variants with associated values (if supported by the source language).
Here's an example of how you could implement a parameterized test:
#[test] fn test_enum_generation_parameterized() { let test_cases = vec![ ( "Empty enum", Composite { type_path: "core::test::EmptyEnum".to_owned(), inners: vec![], r#type: CompositeType::Enum, ..Default::default() }, "", ), ( "Single variant enum", Composite { type_path: "core::test::SingleVariantEnum".to_owned(), inners: vec![CompositeInner { index: 0, name: "Variant".to_owned(), kind: CompositeInnerKind::Key, token: Token::CoreBasic(CoreBasic { type_path: "()".to_owned() }), }], r#type: CompositeType::Enum, ..Default::default() }, "// Type definition for `core::test::SingleVariantEnum` enum\nexport enum SingleVariantEnum {\n\tVariant,\n}\n", ), // Add more test cases here ]; let writer = TsEnumGenerator; for (test_name, token, expected) in test_cases { let mut buff: Vec<String> = Vec::new(); let result = writer.generate(&token, &mut buff).unwrap(); assert_eq!(result, expected, "Failed test case: {}", test_name); } }This approach allows you to easily add more test cases without duplicating the test function.
99-128: Arigato for the helper function, sensei! It's very useful.The
create_available_theme_enum_tokenfunction is well-implemented and provides a good sample enum token for testing. Consider the following suggestions:
- Add a doc comment to explain the purpose of this function.
- Consider parameterizing this function to create different enum tokens easily.
Here's a suggested improvement:
/// Creates a sample enum token with the given name and variants. fn create_sample_enum_token(name: &str, variants: &[&str]) -> Composite { Composite { type_path: format!("core::test::{}", name), inners: variants .iter() .enumerate() .map(|(index, &variant)| CompositeInner { index, name: variant.to_owned(), kind: CompositeInnerKind::Key, token: Token::CoreBasic(CoreBasic { type_path: "()".to_owned() }), }) .collect(), generic_args: vec![], r#type: CompositeType::Enum, is_event: false, alias: None, } } // Usage: let token = create_sample_enum_token("AvailableTheme", &["Light", "Dark", "Dojo"]);This refactored version allows for more flexible creation of sample enum tokens.
crates/dojo-bindgen/src/plugins/typescript/generator/function.rs (1)
114-298: LGTM! Solid implementation and test coverageOhayo, sensei! The implementation of
BindgenContractGeneratorforTsFunctionGeneratorlooks good, and the test coverage is comprehensive. Great job on ensuring thorough testing of the code generation process!One small suggestion: Consider adding a test case for error handling in the generated functions once you've implemented the error rethrowing we discussed earlier. This would ensure that the error propagation works as expected.
crates/dojo-bindgen/src/plugins/typescript/generator/schema.rs (1)
142-274: Consider adding more edge case and error scenario testsOhayo, sensei! The test module provides good coverage of the
TsSchemaGeneratorfunctionality. However, to ensure robustness, consider adding the following test cases:
- Test for handling invalid type paths in
get_namespace_and_path.- Test for handling empty or malformed buffer inputs in
handle_schema_typeandhandle_schema_const.- Test for generating types with nested structs or complex field types.
- Test for error handling in the
generatemethod when given invalid inputs.Adding these test cases will help ensure the generator behaves correctly in various edge cases and error scenarios, improving its overall reliability.
crates/dojo-bindgen/src/plugins/typescript/generator/mod.rs (1)
126-237: Consider enhancing unit test coverage for edge cases.Ohayo, sensei! It would be beneficial to add unit tests for nested arrays, tuples containing arrays, and composite types with nested structures. This will help ensure the robustness of the
JsTypeandJsDefaultValueimplementations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- 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 (1 hunks)
- crates/dojo-bindgen/src/plugins/typescript/generator/mod.rs (1 hunks)
- crates/dojo-bindgen/src/plugins/typescript/generator/schema.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (16)
crates/dojo-bindgen/src/plugins/typescript/generator/interface.rs (4)
1-7: Ohayo, sensei! The imports and struct definition look good.The necessary types and traits are imported, and the
TsInterfaceGeneratorstruct is defined concisely. This sets up a solid foundation for the implementation to follow.
34-72: Ohayo, sensei! These tests are well-crafted and cover important edge cases.The
test_interface_without_innersandtest_interface_not_structfunctions effectively verify that thegeneratemethod returns an empty string when given invalid input. This is crucial for ensuring robust behavior in the generator.
74-82: Ohayo, sensei! Let's improve the readability of this test assertion.As suggested in a previous review, using a raw string for the test assertion would greatly enhance readability and maintainability. Here's how you can implement this change:
assert_eq!(result, r#"// Type definition for `core::test::TestStruct` struct export interface TestStruct { fieldOrder: string[]; field1: number; field2: number; field3: number; } "#);This change will make it easier to update the test in the future if the output format changes.
84-113: 🛠️ Refactor suggestionOhayo, sensei! Let's refactor this helper function to reduce duplication.
As suggested in a previous review, we can improve the
create_test_struct_tokenfunction by introducing a helper function. Here's how you can implement this change:fn create_composite_inner(index: usize, name: &str) -> CompositeInner { CompositeInner { index, name: name.to_owned(), kind: CompositeInnerKind::Key, token: Token::CoreBasic(CoreBasic { type_path: "core::felt252".to_owned() }), } } fn create_test_struct_token() -> Composite { Composite { type_path: "core::test::TestStruct".to_owned(), inners: vec![ create_composite_inner(0, "field1"), create_composite_inner(1, "field2"), create_composite_inner(2, "field3"), ], generic_args: vec![], r#type: CompositeType::Struct, is_event: false, alias: None, } }This refactoring will make the code more maintainable and easier to extend in the future.
crates/dojo-bindgen/src/plugins/typescript/generator/enum.rs (1)
1-6: Ohayo, sensei! LGTM for the struct definition and imports.The
TsEnumGeneratorstruct and its imports are well-defined. The use ofpub(crate)visibility is appropriate for this internal component.crates/dojo-bindgen/src/plugins/typescript/generator/function.rs (6)
1-9: LGTM! Imports and struct definition look good.Ohayo, sensei! The imports cover all necessary modules for the functionality, and the
TsFunctionGeneratorstruct is defined appropriately. Good job on keeping it simple!
52-88: LGTM! Well-implemented input and calldata formattingOhayo, sensei! The
format_function_inputsandformat_function_calldatamethods are well-implemented. They handle different input types appropriately and use functional programming concepts effectively. Good job on creating clean and efficient code!
11-17:⚠️ Potential issueConsider improving import insertion order
Ohayo, sensei! The current implementation of
check_importsmight lead to imports being inserted in reverse order. To ensure they are correctly ordered at the top of the buffer, consider inserting them at position 0 in reverse order.Here's a suggested improvement:
fn check_imports(&self, buffer: &mut Vec<String>) { if !buffer.iter().any(|b| b.contains("import { DojoProvider } from ")) { - buffer.insert(0, "import { DojoProvider } from \"@dojoengine/core\";".to_owned()); - buffer.insert(1, "import { Account } from \"starknet\";".to_owned()); - buffer.insert(2, "import * as models from \"./models.gen\";\n".to_owned()); + buffer.insert(0, "import * as models from \"./models.gen\";\n".to_owned()); + buffer.insert(0, "import { Account } from \"starknet\";".to_owned()); + buffer.insert(0, "import { DojoProvider } from \"@dojoengine/core\";".to_owned()); } }
29-50:⚠️ Potential issueImprove error handling in generated system functions
Ohayo, sensei! The current implementation of
generate_system_functioncatches errors and only logs them. To allow calling code to handle errors appropriately, consider rethrowing the error after logging it.Here's a suggested improvement:
} catch (error) { console.error(error); + throw error; }This change will ensure that errors are both logged and propagated, giving the calling code more control over error handling.
90-95:⚠️ Potential issueConsider improving function body insertion logic
Ohayo, sensei! The current implementation of
append_function_bodymight be fragile, especially as the buffer grows. The calculation of the insertion position could lead to inconsistent results.Consider using a more deterministic method for managing positions in the buffer. For example, you could:
- Keep track of the last inserted function's position.
- Use a structured approach like an Abstract Syntax Tree (AST) for code generation.
This would make the code more robust and less prone to positioning errors as the buffer size changes.
97-111: 🛠️ Refactor suggestionConsider more robust approach for function wrapper generation
Ohayo, sensei! The current implementation of
setup_function_wrapper_endrelies on string manipulation, which can be error-prone and difficult to maintain, especially if the code structure changes.Consider using a code generation library or building the code structure using more robust methods. This could involve:
- Using a TypeScript AST manipulation library.
- Implementing a more structured representation of the generated code.
- Using template strings with placeholders for dynamic content.
These approaches would improve maintainability and reduce the likelihood of bugs introduced by string manipulation.
crates/dojo-bindgen/src/plugins/typescript/generator/schema.rs (3)
1-13: LGTM! Imports and struct definition are well-organized.Ohayo, sensei! The imports and the
TsSchemaGeneratorstruct definition look good. They provide a solid foundation for the implementation that follows.
88-118: LGTM! Thegenerate_type_initmethod is well-implemented.Ohayo, sensei! The
generate_type_initmethod looks good. It correctly generates default values for each field of the struct and handles different token types appropriately.
121-140: LGTM! TheBindgenModelGeneratorimplementation is well-structured.Ohayo, sensei! The
generatemethod in theBindgenModelGeneratorimplementation looks good. It correctly checks for empty inners and non-struct types before proceeding with the generation process. The method follows a logical flow by calling the previously defined helper methods.crates/dojo-bindgen/src/plugins/typescript/generator/mod.rs (2)
8-54: TheJsTypeimplementation looks solid.Ohayo, sensei! The mapping from Rust types to JavaScript types is well-handled, and the recursive handling of arrays and tuples is implemented effectively.
56-124: TheJsDefaultValueimplementation is well-structured.Ohayo, sensei! The default values for various types are appropriately set, and the handling of composite structures is thorough.
| /// Generates the type definition for the schema | ||
| fn handle_schema_type(&self, token: &Composite, buffer: &mut Vec<String>) { | ||
| let (ns, namespace, type_name) = self.get_namespace_and_path(token); | ||
| let schema_type = format!("export interface {namespace}SchemaType extends SchemaType"); | ||
| if !buffer.iter().any(|b| b.contains(&schema_type)) { | ||
| buffer.push(format!("export interface {namespace}SchemaType extends SchemaType {{\n\t{ns}: {{\n\t\t{}: {},\n\t}},\n}}", type_name, type_name)); | ||
| return; | ||
| } | ||
|
|
||
| // type has already been initialized | ||
| let gen = format!("\n\t\t{type_name}: {type_name},"); | ||
| if buffer.iter().any(|b| b.contains(&gen)) { | ||
| return; | ||
| } | ||
|
|
||
| // fastest way to add a field to the interface is to search for the n-1 `,` and add the | ||
| // field directly after it. | ||
| // to improve this logic, we would need to either have some kind of code parsing. | ||
| // we could otherwise have some intermediate representation that we pass to this generator | ||
| // function. | ||
| let pos = buffer.iter().position(|b| b.contains(&schema_type)).unwrap(); | ||
| if let Some(st) = buffer.get_mut(pos) { | ||
| let indices = st.match_indices(",").map(|(i, _)| i).collect::<Vec<usize>>(); | ||
| let append_after = indices[indices.len() - 2] + 1; | ||
| st.insert_str(append_after, &gen); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a code generation library for more robust manipulation
Ohayo, sensei! The current implementation uses string manipulation for code generation, which can be fragile and error-prone. Consider using a code generation library or creating an intermediate representation for more robust code manipulation.
Research and consider implementing a code generation library like syn for Rust or create an intermediate AST-like structure for TypeScript code generation. This would make the code more maintainable and less prone to errors from string manipulation.
Additionally, the handle_schema_type and handle_schema_const methods have similar structures. Consider refactoring these into a more generic method to reduce code duplication.
Also applies to: 61-86
| fn get_namespace_and_path(&self, token: &Composite) -> (String, String, String) { | ||
| let ns_split = token.type_path.split("::").collect::<Vec<&str>>(); | ||
| if ns_split.len() < 2 { | ||
| panic!("type is invalid type_path has to be at least namespace::type"); | ||
| } | ||
| let ns = ns_split[0]; | ||
| let type_name = ns_split[ns_split.len() - 1]; | ||
| let namespace = ns.to_case(Case::Pascal); | ||
| (ns.to_owned(), namespace, type_name.to_owned()) | ||
| } |
There was a problem hiding this comment.
Consider returning a Result instead of panicking
Ohayo, sensei! The get_namespace_and_path method currently panics if the type_path is invalid. It's generally better to return a Result in library code to allow the caller to handle the error gracefully.
Consider refactoring the method to return a Result:
-fn get_namespace_and_path(&self, token: &Composite) -> (String, String, String) {
+fn get_namespace_and_path(&self, token: &Composite) -> Result<(String, String, String), String> {
let ns_split = token.type_path.split("::").collect::<Vec<&str>>();
if ns_split.len() < 2 {
- panic!("type is invalid type_path has to be at least namespace::type");
+ return Err("type is invalid: type_path has to be at least namespace::type".to_string());
}
let ns = ns_split[0];
let type_name = ns_split[ns_split.len() - 1];
let namespace = ns.to_case(Case::Pascal);
- (ns.to_owned(), namespace, type_name.to_owned())
+ Ok((ns.to_owned(), namespace, type_name.to_owned()))
}Don't forget to update the calling methods to handle the Result.
📝 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.
| fn get_namespace_and_path(&self, token: &Composite) -> (String, String, String) { | |
| let ns_split = token.type_path.split("::").collect::<Vec<&str>>(); | |
| if ns_split.len() < 2 { | |
| panic!("type is invalid type_path has to be at least namespace::type"); | |
| } | |
| let ns = ns_split[0]; | |
| let type_name = ns_split[ns_split.len() - 1]; | |
| let namespace = ns.to_case(Case::Pascal); | |
| (ns.to_owned(), namespace, type_name.to_owned()) | |
| } | |
| fn get_namespace_and_path(&self, token: &Composite) -> Result<(String, String, String), String> { | |
| let ns_split = token.type_path.split("::").collect::<Vec<&str>>(); | |
| if ns_split.len() < 2 { | |
| return Err("type is invalid: type_path has to be at least namespace::type".to_string()); | |
| } | |
| let ns = ns_split[0]; | |
| let type_name = ns_split[ns_split.len() - 1]; | |
| let namespace = ns.to_case(Case::Pascal); | |
| Ok((ns.to_owned(), namespace, type_name.to_owned())) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (2)
crates/dojo-bindgen/src/plugins/typescript/generator/function.rs (1)
137-307: Ohayo, sensei! Comprehensive test coverage with room for improvementThe test module provides excellent coverage for the
TsFunctionGeneratorstruct, including tests for individual methods and the overall generation process. The helper functions for creating test data enhance the readability and maintainability of the tests.To further improve the test suite, consider adding a test case for a function with multiple input parameters of different types. This would ensure that the
format_function_inputsmethod handles more complex scenarios correctly.crates/dojo-bindgen/src/plugins/typescript/generator/schema.rs (1)
131-263: LGTM! Comprehensive test coverage with a minor suggestion.Ohayo, sensei! The test module looks great with comprehensive coverage of various scenarios. The use of a helper function
create_test_struct_tokenis a good practice for maintaining consistent test inputs.As a minor suggestion, consider adding a test case for error handling once you've implemented the
Resultreturn type in theget_namespace_and_pathmethod. This will ensure that error cases are properly handled and tested.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- crates/dojo-bindgen/src/plugins/mod.rs (3 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 (1 hunks)
- crates/dojo-bindgen/src/plugins/typescript/generator/schema.rs (1 hunks)
- crates/dojo-bindgen/src/plugins/typescript/writer.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/dojo-bindgen/src/plugins/mod.rs
- crates/dojo-bindgen/src/plugins/typescript/generator/enum.rs
- crates/dojo-bindgen/src/plugins/typescript/generator/interface.rs
🧰 Additional context used
🔇 Additional comments (9)
crates/dojo-bindgen/src/plugins/typescript/writer.rs (2)
13-22: Ohayo, sensei! TheTsFileWriterstruct looks well-structured.The
TsFileWriterstruct and itsnewmethod are well-defined. The use of&'static strfor thepathfield ensures that the path remains valid for the lifetime of the program, which is a good practice for configuration-like data.
82-91: Ohayo, sensei! TheTsFileContractWriterstruct looks well-defined.The structure and initialization of
TsFileContractWriterare clear and concise. The use ofBox<dyn BindgenContractGenerator>allows for flexibility in the types of generators that can be used.crates/dojo-bindgen/src/plugins/typescript/generator/function.rs (2)
56-81: Ohayo, sensei! Well-implemented input formattingThe
format_function_inputsmethod effectively generates a string representation of function input parameters. It handles different token types, including enums, and uses appropriate casing for parameter names. Good job!
118-135: Ohayo, sensei! Well-structured contract generator implementationThe implementation of
BindgenContractGeneratorforTsFunctionGeneratoris well-structured and follows a logical flow. It effectively orchestrates the entire function generation process by calling various helper methods to build the TypeScript code. Good job on maintaining a clear and organized approach!crates/dojo-bindgen/src/plugins/typescript/generator/schema.rs (5)
1-13: LGTM! Imports and struct definition look good.Ohayo, sensei! The imports and struct definition for
TsSchemaGeneratorare well-organized and appropriate for the intended functionality. The use ofpub(crate)for the struct visibility is a good practice for maintaining encapsulation within the crate.
77-129: LGTM! Well-implemented type initialization and generation logic.Ohayo, sensei! The
generate_type_initmethod and theBindgenModelGeneratorimplementation look good. Thegeneratemethod correctly handles edge cases and delegates to other methods for the actual generation work. The use ofBindgenResultfor error handling is a good practice.
1-263: Overall assessment: Solid implementation with room for improvementOhayo, sensei! The
TsSchemaGeneratorimplementation is well-structured and functional. Here's a summary of the key points:
- The overall design and structure of the generator are good.
- Error handling can be improved by replacing panics with
Resultreturns.- Consider using a code generation library or intermediate representation for more robust TypeScript code generation.
- The test coverage is comprehensive, which is excellent for maintaining code quality.
These improvements will make the code more robust, maintainable, and aligned with best practices for library code. Great work overall, and implementing these suggestions will further enhance the quality of this module!
22-31:⚠️ Potential issueConsider returning a Result instead of panicking
Ohayo, sensei! The
get_namespace_and_pathmethod currently panics if thetype_pathis invalid. It's generally better to return aResultin library code to allow the caller to handle the error gracefully.Consider refactoring the method to return a
Result:-fn get_namespace_and_path(&self, token: &Composite) -> (String, String, String) { +fn get_namespace_and_path(&self, token: &Composite) -> Result<(String, String, String), String> { let ns_split = token.type_path.split("::").collect::<Vec<&str>>(); if ns_split.len() < 2 { - panic!("type is invalid type_path has to be at least namespace::type"); + return Err("type is invalid: type_path has to be at least namespace::type".to_string()); } let ns = ns_split[0]; let type_name = ns_split[ns_split.len() - 1]; let namespace = ns.to_case(Case::Pascal); - (ns.to_owned(), namespace, type_name.to_owned()) + Ok((ns.to_owned(), namespace, type_name.to_owned())) }Don't forget to update the calling methods to handle the
Result.
33-75: 🛠️ Refactor suggestionConsider using a code generation library for more robust manipulation
Ohayo, sensei! The current implementation uses string manipulation for code generation, which can be fragile and error-prone. Consider using a code generation library or creating an intermediate representation for more robust code manipulation.
Research and consider implementing a code generation library like
synfor Rust or create an intermediate AST-like structure for TypeScript code generation. This would make the code more maintainable and less prone to errors from string manipulation.Additionally, the
handle_schema_typeandhandle_schema_constmethods have similar structures. Consider refactoring these into a more generic method to reduce code duplication.
| impl BindgenWriter for TsFileWriter { | ||
| fn write(&self, path: &str, data: &DojoData) -> BindgenResult<(PathBuf, Vec<u8>)> { | ||
| let models_path = Path::new(path).to_owned(); | ||
| let mut models = data.models.values().collect::<Vec<_>>(); | ||
|
|
||
| // Sort models based on their tag to ensure deterministic output. | ||
| models.sort_by(|a, b| a.tag.cmp(&b.tag)); | ||
| let composites = models | ||
| .iter() | ||
| .map(|m| { | ||
| let mut composites: Vec<&Composite> = Vec::new(); | ||
| let mut enum_composites = | ||
| m.tokens.enums.iter().map(|e| e.to_composite().unwrap()).collect::<Vec<_>>(); | ||
| let mut struct_composites = | ||
| m.tokens.structs.iter().map(|s| s.to_composite().unwrap()).collect::<Vec<_>>(); | ||
| let mut func_composites = m | ||
| .tokens | ||
| .functions | ||
| .iter() | ||
| .map(|f| f.to_composite().unwrap()) | ||
| .collect::<Vec<_>>(); | ||
| composites.append(&mut enum_composites); | ||
| composites.append(&mut struct_composites); | ||
| composites.append(&mut func_composites); | ||
| composites | ||
| }) | ||
| .flatten() | ||
| .filter(|c| !(c.type_path.starts_with("dojo::") || c.type_path.starts_with("core::"))) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let code = self | ||
| .generators | ||
| .iter() | ||
| .fold(Buffer::new(), |mut acc, g| { | ||
| composites.iter().for_each(|c| { | ||
| match g.generate(c, &mut acc) { | ||
| Ok(code) => { | ||
| if !code.is_empty() { | ||
| acc.push(code) | ||
| } | ||
| } | ||
| Err(_e) => { | ||
| log::error!("Failed to generate code for model {}", c.type_path); | ||
| } | ||
| }; | ||
| }); | ||
| acc | ||
| }) | ||
| .join("\n"); | ||
|
|
||
| Ok((models_path, code.as_bytes().to_vec())) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Consider refactoring the write method for better readability and error handling.
The write method is quite complex and could benefit from some refactoring:
- The
unwrap()calls on lines 36, 38, and 43 could lead to panics. Consider usingfilter_mapto handle potential errors gracefully:
let enum_composites = m.tokens.enums.iter()
.filter_map(|e| e.to_composite().ok())
.collect::<Vec<_>>();
// Apply similar changes for struct_composites and func_composites-
The nested closures and iterator chains make the code hard to follow. Consider extracting some of this logic into separate methods for better readability.
-
The error handling in the generator loop (lines 59-68) is good, but you might want to consider collecting all errors and returning them, rather than just logging them.
-
The
filteron line 51 could be moved earlier in the chain to avoid unnecessary processing of filtered-out items.
Would you like assistance in implementing these refactoring suggestions?
| impl BindgenWriter for TsFileContractWriter { | ||
| fn write(&self, path: &str, data: &DojoData) -> BindgenResult<(PathBuf, Vec<u8>)> { | ||
| let models_path = Path::new(path).to_owned(); | ||
|
|
||
| let code = self | ||
| .generators | ||
| .iter() | ||
| .fold(Buffer::new(), |mut acc, g| { | ||
| data.contracts.iter().for_each(|(_, c)| { | ||
| c.systems | ||
| .iter() | ||
| .filter(|s| { | ||
| let name = s.to_function().unwrap().name.as_str(); | ||
| ![ | ||
| "contract_name", | ||
| "namespace", | ||
| "tag", | ||
| "name_hash", | ||
| "selector", | ||
| "dojo_init", | ||
| "namespace_hash", | ||
| "world", | ||
| ] | ||
| .contains(&name) | ||
| }) | ||
| .for_each(|s| match s.to_function() { | ||
| Ok(f) => match g.generate(c, f, &mut acc) { | ||
| Ok(code) => { | ||
| if !code.is_empty() { | ||
| acc.push(code) | ||
| } | ||
| } | ||
| Err(_) => { | ||
| log::error!("Failed to generate code for system {:?}", s); | ||
| } | ||
| }, | ||
| Err(_) => { | ||
| log::error!("Failed to get function out of system {:?}", s); | ||
| } | ||
| }) | ||
| }); | ||
|
|
||
| acc | ||
| }) | ||
| .join("\n"); | ||
| Ok((models_path, code.as_bytes().to_vec())) | ||
| } | ||
| fn get_path(&self) -> &str { | ||
| self.path | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! The write method could use some refinements for better error handling and readability.
- The
unwrap()call on line 105 could lead to panics. Consider using amatchorif letto handle theResult:
if let Ok(function) = s.to_function() {
let name = function.name.as_str();
// ... rest of the filtering logic
} else {
log::error!("Failed to get function out of system {:?}", s);
return;
}-
The filtering logic (lines 104-117) could be extracted into a separate function for better readability.
-
The error handling in the generator loop (lines 119-132) is good, but consider collecting all errors and returning them, rather than just logging them.
-
The
matchstatement fors.to_function()(lines 118-132) could be simplified by usingand_then:
.filter_map(|s| s.to_function().ok())
.for_each(|f| {
if let Err(_) = g.generate(c, f, &mut acc) {
log::error!("Failed to generate code for system {:?}", f);
}
});Would you like assistance in implementing these refactoring suggestions?
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use crate::{DojoData, DojoWorld}; | ||
| use std::{collections::HashMap, path::PathBuf}; | ||
|
|
||
| #[test] | ||
| fn test_ts_file_writer() { | ||
| let writer = TsFileWriter::new("models.gen.ts", Vec::new()); | ||
|
|
||
| let data = DojoData { | ||
| models: HashMap::new(), | ||
| contracts: HashMap::new(), | ||
| world: DojoWorld { name: "0x01".to_string() }, | ||
| }; | ||
|
|
||
| let (path, code) = writer.write("models.gen.ts", &data).unwrap(); | ||
| assert_eq!(path, PathBuf::from("models.gen.ts")); | ||
| assert_eq!(code, Vec::<u8>::new()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Let's enhance our test coverage for more robust validation.
The current test is a good start, but it only covers the most basic scenario. Consider adding the following tests:
- Test with non-empty
DojoDatato ensure proper code generation. - Test with multiple generators to verify correct aggregation of generated code.
- Test error handling scenarios, such as invalid input data or generator failures.
- Add similar tests for
TsFileContractWriter.
Example:
#[test]
fn test_ts_file_writer_with_data() {
// Create a mock generator
struct MockGenerator;
impl BindgenModelGenerator for MockGenerator {
fn generate(&self, _: &Composite, _: &mut Buffer) -> BindgenResult<String> {
Ok("mock generated code".to_string())
}
}
let writer = TsFileWriter::new("models.gen.ts", vec![Box::new(MockGenerator)]);
// Create non-empty DojoData
let mut models = HashMap::new();
// Add some mock models...
let data = DojoData {
models,
contracts: HashMap::new(),
world: DojoWorld { name: "0x01".to_string() },
};
let (path, code) = writer.write("models.gen.ts", &data).unwrap();
assert_eq!(path, PathBuf::from("models.gen.ts"));
assert!(!code.is_empty());
assert_eq!(String::from_utf8(code).unwrap(), "mock generated code");
}Would you like assistance in implementing these additional tests?
|
@MartianGreed, generated components should also include the new Torii indexed ERC20/ERC721 models, so they can be synched by the Torii client, implemented on #2494 These are the models, from graphQL schema: type ERC__Balance {
balance: String!
type: String!
tokenMetadata: ERC__Token!
}
type ERC__Token {
name: String!
symbol: String!
tokenId: String!
decimals: String!
contractAddress: String!
}
type ERC__Transfer {
from: String!
to: String!
amount: String!
type: String!
executedAt: String!
tokenMetadata: ERC__Token!
transactionHash: String!
} |
|
@rsodre do you want to query those models over grpc or over graphql ? Or maybe both ? |
|
those are already available on GraphQL, example here. |
|
@MartianGreed will get back to it shortly and let a review. Thanks for your patience. 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (6)
crates/dojo-bindgen/src/plugins/typescript/mod.rs (2)
20-22: Ohayo, sensei! Great improvement to theTypescriptPluginstruct!The addition of the
writersfield as aVec<Box<dyn BindgenWriter>>is a smart move. It allows for a more flexible and extensible plugin architecture, enabling polymorphic behavior for different types of writers.Consider adding a brief comment explaining the purpose of the
writersfield for improved code documentation:pub struct TypescriptPlugin { + /// Collection of writers responsible for generating different parts of the TypeScript code writers: Vec<Box<dyn BindgenWriter>>, }
25-42: Ohayo, sensei! Excellent initialization of writers in thenewmethod!The updated
newmethod now initializes thewritersfield withTsFileWriterandTsFileContractWriter, each associated with specific generators. This approach provides great flexibility for adding or removing writers and generators.To further enhance flexibility, consider extracting the writer initialization into a separate method:
impl TypescriptPlugin { pub fn new() -> Self { Self { writers: Self::initialize_writers(), } } fn initialize_writers() -> Vec<Box<dyn BindgenWriter>> { vec![ Box::new(TsFileWriter::new( "models.gen.ts", vec![ Box::new(TsInterfaceGenerator {}), Box::new(TsEnumGenerator {}), Box::new(TsSchemaGenerator {}), Box::new(TsErcGenerator {}), ], )), Box::new(TsFileContractWriter::new( "contracts.gen.ts", vec![Box::new(TsFunctionGenerator {})], )), ] } }This refactoring would make it easier to modify the writer initialization logic in the future without cluttering the
newmethod.crates/dojo-bindgen/src/plugins/mod.rs (1)
12-12: Ohayo, sensei! New module alert!I see you've added a shiny new
recsmodule. While it's exciting to see new features, it would be super helpful to have a brief comment explaining what this module is all about. What kind of records or recommendations does it handle? A little documentation goes a long way in helping fellow coders understand your vision!Could you add a quick comment above this line to explain the purpose of the
recsmodule?crates/dojo-bindgen/src/plugins/typescript/generator/schema.rs (1)
94-229: LGTM! Comprehensive test coverage.Ohayo, sensei! The test module provides excellent coverage of the
TsSchemaGenerator's functionality. The tests cover various scenarios, including empty structs, import generation, schema type generation, and schema const generation.One suggestion to further improve the test suite: consider adding a test case for a struct with different field types (not just
felt252) to ensure the generator handles various Rust types correctly when generating TypeScript schemas.Would you like assistance in crafting an additional test case with diverse field types?
crates/dojo-bindgen/src/plugins/typescript/generator/function.rs (1)
1-300: Ohayo, sensei! Overall assessment of the TypeScript function generatorThe
TsFunctionGeneratorimplementation provides a comprehensive solution for generating TypeScript functions from Dojo contracts. The code is generally well-structured and functional. However, there are several areas where improvements can be made to enhance robustness, maintainability, and error handling:
- Import insertion logic in
check_imports- Error handling in generated system functions
- Buffer management in
append_function_body- Return statement management in
setup_function_wrapper_end- Clarification on the empty string return in the
generatemethodAddressing these points will significantly improve the code quality and make it more resilient to future changes. Great work overall, and I look forward to seeing the enhancements!
crates/dojo-bindgen/src/plugins/typescript/generator/mod.rs (1)
170-330: Enhance test coverage with additional scenariosOhayo, sensei! The test module looks comprehensive, but we can further improve it by adding a few more test cases:
- Test error handling in
get_namespace_and_path(after implementing the suggestedResultreturn type):#[test] fn test_get_namespace_and_path_error() { let invalid_token = Composite { type_path: "invalid".to_owned(), // ... other fields ... }; assert!(get_namespace_and_path(&invalid_token).is_err()); }
- Test custom type handling in
JsTypeandJsDefaultValue:#[test] fn test_custom_type() { let custom_token = Token::Composite(Composite { type_path: "custom::MyType".to_owned(), // ... other fields ... }); assert_eq!("CustomMyType", JsType::from(&custom_token).to_string()); assert_eq!("new CustomMyType()", JsDefaultValue::from(&custom_token).to_string()); }
- Test the
generate_type_initfunction with a more complex struct containing nested types:#[test] fn test_generate_type_init_complex() { let token = create_complex_test_struct_token(); let init_type = generate_type_init(&token); // Assert the expected complex structure // ... }Adding these tests will increase confidence in the code's behavior across various scenarios.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- crates/dojo-bindgen/src/plugins/mod.rs (3 hunks)
- crates/dojo-bindgen/src/plugins/typescript/generator/enum.rs (1 hunks)
- crates/dojo-bindgen/src/plugins/typescript/generator/erc.rs (1 hunks)
- crates/dojo-bindgen/src/plugins/typescript/generator/function.rs (1 hunks)
- crates/dojo-bindgen/src/plugins/typescript/generator/interface.rs (1 hunks)
- crates/dojo-bindgen/src/plugins/typescript/generator/mod.rs (1 hunks)
- crates/dojo-bindgen/src/plugins/typescript/generator/schema.rs (1 hunks)
- crates/dojo-bindgen/src/plugins/typescript/mod.rs (1 hunks)
- crates/dojo-bindgen/src/plugins/typescript/writer.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/dojo-bindgen/src/plugins/typescript/generator/enum.rs
- crates/dojo-bindgen/src/plugins/typescript/generator/interface.rs
🧰 Additional context used
🔇 Additional comments (19)
crates/dojo-bindgen/src/plugins/typescript/mod.rs (3)
5-10: Ohayo, sensei! Excellent modularization of code generation components!The addition of new imports and modules for generators and writers demonstrates a commendable effort to improve code organization and separation of concerns. This modular approach will enhance maintainability and extensibility of the codebase.
Also applies to: 17-18
Line range hint
1-72: Ohayo, sensei! Excellent overall structure and organizationThe file is well-organized with a clear separation of imports, struct definition, and implementation. It follows Rust conventions and best practices, making it easy to read and maintain. Great job on maintaining a clean and structured codebase!
Line range hint
1-72: Ohayo, sensei! Impressive refactoring of the TypescriptPluginThis refactoring represents a significant improvement in the plugin's architecture. The introduction of modular components for code generation, the flexible writer system, and the overall clean structure are commendable.
Key improvements:
- Enhanced modularity with separate generator and writer components.
- Flexible plugin architecture using a vector of
BindgenWritertrait objects.- Clear separation of concerns in code generation.
While the changes are largely positive, there are a few minor optimizations suggested in previous comments that could further enhance the code:
- Improving error handling in the
generate_codemethod.- Optimizing the code insertion process to avoid unnecessary cloning.
- Extracting the writer initialization into a separate method for better maintainability.
Overall, this is a well-executed refactoring that significantly improves the extensibility and maintainability of the TypeScript plugin. Great work, sensei!
crates/dojo-bindgen/src/plugins/mod.rs (7)
3-3: Ohayo, sensei! New import looks good!The addition of
DerefandDerefMuttraits from the standard library is a smart move. It suggests we're about to see some cool custom type that behaves like a built-in collection. Nice thinking ahead!
7-7: Ohayo again, sensei! Import update is on point!Adding
Functionto the import fromcainome::parser::tokensis a solid move. It's clear you're gearing up for some advanced contract generation magic. Keep that coding fu strong!
10-10: Ohayo once more, sensei! Import game strong!Adding
DojoContractto the import alongsideDojoDatashows you're thinking ahead. This paves the way for some serious contract generation wizardry later on. Excellent foresight!
22-22: Ohayo, sensei! Enum update looking sharp!The addition of the
Recsvariant toBuiltinPluginsand its correspondingfmt::Displayimplementation is spot on. Great job keeping everything in sync!Just a friendly reminder: Don't forget to update any associated documentation for
BuiltinPluginsto include information about the newRecsvariant. This will help keep everything crystal clear for our fellow code ninjas!Also applies to: 31-31
36-73: Ohayo, sensei! Buffer struct is a work of art!The new
Bufferstruct is a brilliant addition to our codebase. It's like aVec<String>on steroids! Here's what I love about it:
- The utility methods like
has,push, andinsert_afteradd some serious firepower to string manipulation.- Implementing
DerefandDerefMutis a pro move, allowingBufferto be used just like aVec<String>. Very smooth!- The
joinmethod is a nice touch for easy string concatenation.Overall, this is a well-thought-out and implemented struct that's going to make our code much cleaner and more efficient. Excellent work, sensei!
85-114: Ohayo, sensei! New traits looking mighty fine!The introduction of
BindgenWriter,BindgenModelGenerator, andBindgenContractGeneratortraits is a fantastic addition to our bindgen arsenal. Here's what I love:
- The
Syncbound on all traits ensures thread safety. Nice thinking!- The separation of concerns between writing, model generation, and contract generation is spot on.
- The use of
Bufferin the new traits ties in nicely with theBufferstruct we saw earlier.While the current documentation is a good start, I echo the previous suggestion to beef up the docs a bit more. Some ideas:
- Add examples of how these traits might be implemented.
- Explain the relationship between these traits and how they fit into the larger bindgen process.
- Clarify any constraints or expectations for implementations of these traits.
Remember, great documentation is like a well-commented kata - it helps fellow coders understand and master the technique!
Keep up the awesome work, sensei! This PR is shaping up to be a real game-changer for our bindgen capabilities.
76-76: Ohayo, sensei! Trait upgrade detected!Adding the
Syncbound toBuiltinPluginis a smart move. It ensures thread safety for our plugin implementations, which is crucial for robust concurrent operations. Nicely done!Just a heads-up: This change might affect existing implementations of
BuiltinPlugin. It would be wise to double-check that all current implementations satisfy theSyncrequirement. Here's a quick script to help verify:This will help us catch any potential issues early on. Keep up the great work, sensei!
✅ Verification successful
Ohayo, sensei! All systems are a go!
Checked all
BuiltinPluginimplementations, and they all comply with the newSyncrequirement. Everything looks thread-safe and ready for action.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all implementations of BuiltinPlugin and check if they implement Sync echo "Searching for BuiltinPlugin implementations..." rg --type rust "impl.*BuiltinPlugin.*for" -A 5 echo "\nChecking if these types implement Sync..." rg --type rust "impl.*Sync.*for"Length of output: 3396
crates/dojo-bindgen/src/plugins/typescript/writer.rs (3)
9-18: Ohayo, sensei! TheTsFileWriterstruct looks good!The implementation of
TsFileWriterand itsnewmethod is clean and straightforward. It provides a clear way to initialize the writer with a path and a list of generators.
72-74: Ohayo, sensei! Theget_pathmethod looks perfect!This method is simple and does exactly what it's supposed to do - return the
pathfield. No changes needed here.
77-86: Ohayo, sensei! TheTsFileContractWriterstruct is well-crafted!The implementation of
TsFileContractWriterand itsnewmethod follows the same pattern asTsFileWriter, which is good for consistency. It provides a clear way to initialize the contract writer with a path and a list of generators.crates/dojo-bindgen/src/plugins/typescript/generator/schema.rs (3)
11-11: LGTM! Simple and effective struct definition.Ohayo, sensei! The
TsSchemaGeneratorstruct is defined without any fields, which is appropriate for its purpose as a stateless generator. This design allows for a clean implementation of methods and traits.
14-18: LGTM! Efficient import handling.Ohayo, sensei! The
import_schema_typemethod efficiently ensures that theSchemaTypeimport is present only once in the buffer. This approach prevents duplicate imports and maintains clean code output.
70-89: LGTM! Well-structured trait implementation.Ohayo, sensei! The implementation of
BindgenModelGeneratorforTsSchemaGeneratoris well-structured. It correctly handles edge cases (empty or non-struct tokens) and delegates the actual generation work to the appropriate methods. This design promotes good separation of concerns and maintainability.crates/dojo-bindgen/src/plugins/typescript/generator/function.rs (3)
20-28: Ohayo, sensei! LGTM forsetup_function_wrapper_startThe implementation of
setup_function_wrapper_startis efficient and correct. It avoids duplication by checking if the wrapper already exists and provides the necessary index for later use.
53-89: Ohayo, sensei! LGTM for input formatting methodsThe implementations of
format_function_inputsandformat_function_calldataare well-structured and handle the formatting appropriately. Theformat_function_inputsmethod correctly handles enum types from models, which is a nice touch.
115-132: Ohayo, sensei! Question about the empty string return ingenerateThe
generatemethod of theBindgenContractGeneratorimplementation looks correct and uses the defined methods appropriately. However, it returns an empty string (Ok(String::new())), which might not be the intended behavior.Could you clarify the purpose of returning an empty string here? If this is intentional, consider adding a comment explaining why. If not, you might want to return a more meaningful value, such as the generated code or a success message.
Here's a potential improvement if returning the generated code is desired:
- Ok(String::new()) + Ok(buffer.join("\n"))This would return the entire generated buffer as a single string.
| code.iter().for_each(|(path, code)| { | ||
| if code.is_empty() { | ||
| return; | ||
| } | ||
| out.insert(PathBuf::from(path), code.clone()); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Let's optimize the code insertion process
The current implementation correctly checks for empty code before insertion, which is good. However, we can further optimize this process to avoid unnecessary cloning and improve performance.
Consider using into_iter() instead of iter() and remove the clone() call:
- code.iter().for_each(|(path, code)| {
+ code.into_iter().for_each(|(path, code)| {
if code.is_empty() {
return;
}
- out.insert(PathBuf::from(path), code.clone());
+ out.insert(PathBuf::from(path), code);
});This change avoids unnecessary cloning of the code and should improve performance, especially for large code generations.
📝 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.
| code.iter().for_each(|(path, code)| { | |
| if code.is_empty() { | |
| return; | |
| } | |
| out.insert(PathBuf::from(path), code.clone()); | |
| }); | |
| code.into_iter().for_each(|(path, code)| { | |
| if code.is_empty() { | |
| return; | |
| } | |
| out.insert(PathBuf::from(path), code); | |
| }); |
| let code = self | ||
| .writers | ||
| .iter() | ||
| .map(|writer| match writer.write(writer.get_path(), data) { | ||
| Ok(c) => c, | ||
| Err(e) => { | ||
| log::error!("Failed to generate code for typescript plugin: {e}"); | ||
| ("".into(), Vec::new()) | ||
| } | ||
| }) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
Ohayo, sensei! Let's enhance the error handling in generate_code
The refactoring of the generate_code method to use the new writers field is a great improvement for modularity. However, there's room for improvement in error handling and preventing potential issues with empty paths and codes.
Consider modifying the error handling to skip over writers that fail, preventing entries with empty paths from being inserted into the output map. Apply this diff to improve error handling:
let code = self
.writers
.iter()
- .map(|writer| match writer.write(writer.get_path(), data) {
+ .filter_map(|writer| match writer.write(writer.get_path(), data) {
Ok(c) => Some(c),
Err(e) => {
log::error!("Failed to generate code for typescript plugin: {e}");
- ("".into(), Vec::new())
+ None
}
})
.collect::<Vec<_>>();This change ensures that only successful code generations are included in the output, preventing potential issues with empty paths and codes.
📝 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.
| let code = self | |
| .writers | |
| .iter() | |
| .map(|writer| match writer.write(writer.get_path(), data) { | |
| Ok(c) => c, | |
| Err(e) => { | |
| log::error!("Failed to generate code for typescript plugin: {e}"); | |
| ("".into(), Vec::new()) | |
| } | |
| }) | |
| .collect::<Vec<_>>(); | |
| let code = self | |
| .writers | |
| .iter() | |
| .filter_map(|writer| match writer.write(writer.get_path(), data) { | |
| Ok(c) => Some(c), | |
| Err(e) => { | |
| log::error!("Failed to generate code for typescript plugin: {e}"); | |
| None | |
| } | |
| }) | |
| .collect::<Vec<_>>(); |
| fn write(&self, path: &str, data: &DojoData) -> BindgenResult<(PathBuf, Vec<u8>)> { | ||
| let models_path = Path::new(path).to_owned(); | ||
| let mut models = data.models.values().collect::<Vec<_>>(); | ||
|
|
||
| // Sort models based on their tag to ensure deterministic output. | ||
| models.sort_by(|a, b| a.tag.cmp(&b.tag)); | ||
| let composites = models | ||
| .iter() | ||
| .flat_map(|m| { | ||
| let mut composites: Vec<&Composite> = Vec::new(); | ||
| let mut enum_composites = | ||
| m.tokens.enums.iter().map(|e| e.to_composite().unwrap()).collect::<Vec<_>>(); | ||
| let mut struct_composites = | ||
| m.tokens.structs.iter().map(|s| s.to_composite().unwrap()).collect::<Vec<_>>(); | ||
| let mut func_composites = m | ||
| .tokens | ||
| .functions | ||
| .iter() | ||
| .map(|f| f.to_composite().unwrap()) | ||
| .collect::<Vec<_>>(); | ||
| composites.append(&mut enum_composites); | ||
| composites.append(&mut struct_composites); | ||
| composites.append(&mut func_composites); | ||
| composites | ||
| }) | ||
| .filter(|c| !(c.type_path.starts_with("dojo::") || c.type_path.starts_with("core::"))) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let code = self | ||
| .generators | ||
| .iter() | ||
| .fold(Buffer::new(), |mut acc, g| { | ||
| composites.iter().for_each(|c| { | ||
| match g.generate(c, &mut acc) { | ||
| Ok(code) => { | ||
| if !code.is_empty() { | ||
| acc.push(code) | ||
| } | ||
| } | ||
| Err(_e) => { | ||
| log::error!("Failed to generate code for model {}", c.type_path); | ||
| } | ||
| }; | ||
| }); | ||
| acc | ||
| }) | ||
| .join("\n"); | ||
|
|
||
| Ok((models_path, code.as_bytes().to_vec())) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Let's enhance the robustness and readability of the write method.
The write method is comprehensive, but there are a few areas we can improve:
- Replace
unwrap()calls withfilter_mapto handle potential errors gracefully:
let enum_composites = m.tokens.enums.iter()
.filter_map(|e| e.to_composite().ok())
.collect::<Vec<_>>();
// Apply similar changes for struct_composites and func_composites-
Consider returning errors instead of just logging them. This allows the caller to handle errors more flexibly.
-
The nested closures and iterator chains make the code hard to follow. Consider extracting some of this logic into separate methods for better readability.
-
The
filteron line 46 could be moved earlier in the chain to avoid unnecessary processing of filtered-out items.
These changes will make the code more robust and easier to maintain.
| fn write(&self, path: &str, data: &DojoData) -> BindgenResult<(PathBuf, Vec<u8>)> { | ||
| let models_path = Path::new(path).to_owned(); | ||
|
|
||
| let code = self | ||
| .generators | ||
| .iter() | ||
| .fold(Buffer::new(), |mut acc, g| { | ||
| data.contracts.iter().for_each(|(_, c)| { | ||
| c.systems | ||
| .iter() | ||
| .filter(|s| { | ||
| let name = s.to_function().unwrap().name.as_str(); | ||
| ![ | ||
| "contract_name", | ||
| "namespace", | ||
| "tag", | ||
| "name_hash", | ||
| "selector", | ||
| "dojo_init", | ||
| "namespace_hash", | ||
| "world", | ||
| ] | ||
| .contains(&name) | ||
| }) | ||
| .for_each(|s| match s.to_function() { | ||
| Ok(f) => match g.generate(c, f, &mut acc) { | ||
| Ok(code) => { | ||
| if !code.is_empty() { | ||
| acc.push(code) | ||
| } | ||
| } | ||
| Err(_) => { | ||
| log::error!("Failed to generate code for system {:?}", s); | ||
| } | ||
| }, | ||
| Err(_) => { | ||
| log::error!("Failed to get function out of system {:?}", s); | ||
| } | ||
| }) | ||
| }); | ||
|
|
||
| acc | ||
| }) | ||
| .join("\n"); | ||
| Ok((models_path, code.as_bytes().to_vec())) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Let's refine the write method for TsFileContractWriter.
The write method for TsFileContractWriter is comprehensive but could benefit from some improvements:
- Replace the
unwrap()call on line 100 with amatchorif letto handle theResult:
if let Ok(function) = s.to_function() {
let name = function.name.as_str();
// ... rest of the filtering logic
} else {
log::error!("Failed to get function out of system {:?}", s);
return;
}-
Consider collecting all errors and returning them, rather than just logging them. This allows the caller to handle errors more flexibly.
-
The filtering logic (lines 99-112) could be extracted into a separate function for better readability.
-
The
matchstatement fors.to_function()(lines 113-127) could be simplified by usingand_then:
.filter_map(|s| s.to_function().ok())
.for_each(|f| {
if let Err(_) = g.generate(c, f, &mut acc) {
log::error!("Failed to generate code for system {:?}", f);
}
});These changes will improve the robustness and readability of the code.
| #[cfg(test)] | ||
| mod tests { | ||
| use std::collections::HashMap; | ||
| use std::path::PathBuf; | ||
|
|
||
| use super::*; | ||
| use crate::{DojoData, DojoWorld}; | ||
|
|
||
| #[test] | ||
| fn test_ts_file_writer() { | ||
| let writer = TsFileWriter::new("models.gen.ts", Vec::new()); | ||
|
|
||
| let data = DojoData { | ||
| models: HashMap::new(), | ||
| contracts: HashMap::new(), | ||
| world: DojoWorld { name: "0x01".to_string() }, | ||
| }; | ||
|
|
||
| let (path, code) = writer.write("models.gen.ts", &data).unwrap(); | ||
| assert_eq!(path, PathBuf::from("models.gen.ts")); | ||
| assert_eq!(code, Vec::<u8>::new()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Let's expand our test coverage for more robust validation.
The current test is a good start, but we can enhance it to cover more scenarios:
- Test with non-empty
DojoDatato ensure proper code generation. - Test with multiple generators to verify correct aggregation of generated code.
- Test error handling scenarios, such as invalid input data or generator failures.
- Add similar tests for
TsFileContractWriter.
Here's an example of an additional test:
#[test]
fn test_ts_file_writer_with_data() {
// Create a mock generator
struct MockGenerator;
impl BindgenModelGenerator for MockGenerator {
fn generate(&self, _: &Composite, _: &mut Buffer) -> BindgenResult<String> {
Ok("mock generated code".to_string())
}
}
let writer = TsFileWriter::new("models.gen.ts", vec![Box::new(MockGenerator)]);
// Create non-empty DojoData
let mut models = HashMap::new();
// Add some mock models...
let data = DojoData {
models,
contracts: HashMap::new(),
world: DojoWorld { name: "0x01".to_string() },
};
let (path, code) = writer.write("models.gen.ts", &data).unwrap();
assert_eq!(path, PathBuf::from("models.gen.ts"));
assert!(!code.is_empty());
assert_eq!(String::from_utf8(code).unwrap(), "mock generated code");
}These additional tests will help ensure the robustness of our code.
| pub(crate) fn generate_type_init(token: &Composite) -> String { | ||
| format!( | ||
| "{{\n\t\t\tfieldOrder: [{}],\n{}\n\t\t}}", | ||
| token.inners.iter().map(|i| format!("'{}'", i.name)).collect::<Vec<String>>().join(", "), | ||
| token | ||
| .inners | ||
| .iter() | ||
| .map(|i| { | ||
| match i.token.to_composite() { | ||
| Ok(c) => { | ||
| format!("\t\t\t{}: {},", i.name, JsDefaultValue::from(c)) | ||
| } | ||
| Err(_) => { | ||
| // this will fail on core types as | ||
| // `core::starknet::contract_address::ContractAddress` | ||
| // `core::felt252` | ||
| // `core::integer::u64` | ||
| // and so son | ||
| format!("\t\t\t{}: {},", i.name, JsDefaultValue::from(&i.token)) | ||
| } | ||
| } | ||
| }) | ||
| .collect::<Vec<String>>() | ||
| .join("\n") | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance readability with comments and consider refactoring
Ohayo, sensei! The generate_type_init function is doing a lot of heavy lifting. To improve its maintainability, consider adding more detailed comments explaining the logic, especially for the complex iterator chains. Also, you might want to refactor some parts into smaller, named functions for better readability.
For example:
pub(crate) fn generate_type_init(token: &Composite) -> String {
let field_order = generate_field_order(token);
let field_initializations = generate_field_initializations(token);
format!(
"{{\n\t\t\tfieldOrder: [{}],\n{}\n\t\t}}",
field_order,
field_initializations
)
}
fn generate_field_order(token: &Composite) -> String {
token.inners.iter()
.map(|i| format!("'{}'", i.name))
.collect::<Vec<String>>()
.join(", ")
}
fn generate_field_initializations(token: &Composite) -> String {
// ... implementation ...
}This approach would make the main function easier to understand at a glance.
| #[derive(Debug)] | ||
| pub(crate) struct JsType(String); | ||
| impl From<&str> for JsType { | ||
| fn from(value: &str) -> Self { | ||
| match value { | ||
| "felt252" => JsType("number".to_owned()), | ||
| "ContractAddress" => JsType("string".to_owned()), | ||
| "ByteArray" => JsType("string".to_owned()), | ||
| "u8" => JsType("number".to_owned()), | ||
| "u16" => JsType("number".to_owned()), | ||
| "u32" => JsType("number".to_owned()), | ||
| "u64" => JsType("number".to_owned()), | ||
| "u128" => JsType("number".to_owned()), | ||
| "u256" => JsType("number".to_owned()), | ||
| "U256" => JsType("number".to_owned()), | ||
| "bool" => JsType("boolean".to_owned()), | ||
| _ => JsType(value.to_owned()), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<&Token> for JsType { | ||
| fn from(value: &Token) -> Self { | ||
| match value { | ||
| Token::Array(a) => JsType::from(format!("Array<{}>", JsType::from(&*a.inner)).as_str()), | ||
| Token::Tuple(t) => JsType::from( | ||
| format!( | ||
| "[{}]", | ||
| t.inners | ||
| .iter() | ||
| .map(|i| JsType::from(i.type_name().as_str()).to_string()) | ||
| .collect::<Vec<_>>() | ||
| .join(", ") | ||
| .as_str() | ||
| ) | ||
| .as_str(), | ||
| ), | ||
| _ => JsType::from(value.type_name().as_str()), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl std::fmt::Display for JsType { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| f.write_str(&self.0) | ||
| } | ||
| } |
There was a problem hiding this comment.
Review and update type mappings for consistency
Ohayo, sensei! The JsType struct and its implementations look good overall, but there are a few points to consider:
- The mapping for
u64,u128, andu256to JavaScript'snumbertype might lead to precision loss. Consider usingBigIntfor these types:
- "u64" => JsType("number".to_owned()),
- "u128" => JsType("number".to_owned()),
- "u256" => JsType("number".to_owned()),
+ "u64" => JsType("BigInt".to_owned()),
+ "u128" => JsType("BigInt".to_owned()),
+ "u256" => JsType("BigInt".to_owned()),- The
ByteArrayis mapped tostring, which might not be the best representation. Consider usingUint8Arrayinstead:
- "ByteArray" => JsType("string".to_owned()),
+ "ByteArray" => JsType("Uint8Array".to_owned()),- You might want to add a catch-all case for custom types, returning them as-is or with a specific prefix:
_ => JsType(format!("Custom{}", value.to_owned())),These changes would improve type safety and consistency in the generated TypeScript code.
| #[derive(Debug)] | ||
| pub(crate) struct JsDefaultValue(String); | ||
| impl From<&str> for JsDefaultValue { | ||
| fn from(value: &str) -> Self { | ||
| match value { | ||
| "felt252" => JsDefaultValue("0".to_string()), | ||
| "ContractAddress" => JsDefaultValue("\"\"".to_string()), | ||
| "ByteArray" => JsDefaultValue("\"\"".to_string()), | ||
| "u8" => JsDefaultValue("0".to_string()), | ||
| "u16" => JsDefaultValue("0".to_string()), | ||
| "u32" => JsDefaultValue("0".to_string()), | ||
| "u64" => JsDefaultValue("0".to_string()), | ||
| "u128" => JsDefaultValue("0".to_string()), | ||
| "u256" => JsDefaultValue("0".to_string()), | ||
| "U256" => JsDefaultValue("0".to_string()), | ||
| "bool" => JsDefaultValue("false".to_string()), | ||
| _ => JsDefaultValue(value.to_string()), | ||
| } | ||
| } | ||
| } | ||
| impl From<&Composite> for JsDefaultValue { | ||
| fn from(value: &Composite) -> Self { | ||
| match value.r#type { | ||
| cainome::parser::tokens::CompositeType::Enum => { | ||
| JsDefaultValue(format!("{}.{}", value.type_name(), value.inners[0].name)) | ||
| } | ||
| cainome::parser::tokens::CompositeType::Struct => JsDefaultValue(format!( | ||
| "{{ {} }}", | ||
| value | ||
| .inners | ||
| .iter() | ||
| .map(|i| format!("{}: {},", i.name, JsDefaultValue::from(&i.token))) | ||
| .collect::<Vec<_>>() | ||
| .join("\n") | ||
| )), | ||
| _ => JsDefaultValue::from(value.type_name().as_str()), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<&Token> for JsDefaultValue { | ||
| fn from(value: &Token) -> Self { | ||
| match value { | ||
| Token::Array(a) => { | ||
| JsDefaultValue::from(format!("[{}]", JsDefaultValue::from(&*a.inner)).as_str()) | ||
| } | ||
| Token::Tuple(t) => JsDefaultValue::from( | ||
| format!( | ||
| "[{}]", | ||
| t.inners | ||
| .iter() | ||
| .map(|i| JsDefaultValue::from(i.type_name().as_str()).to_string()) | ||
| .collect::<Vec<_>>() | ||
| .join(", ") | ||
| .as_str() | ||
| ) | ||
| .as_str(), | ||
| ), | ||
| Token::Composite(c) => JsDefaultValue::from(c), | ||
| _ => JsDefaultValue::from(value.type_name().as_str()), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl std::fmt::Display for JsDefaultValue { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| f.write_str(&self.0) | ||
| } | ||
| } |
There was a problem hiding this comment.
Align default values with updated type mappings
Ohayo, sensei! The JsDefaultValue struct and its implementations look good, but we should align them with the suggested changes for JsType:
- Update default values for
u64,u128, andu256to useBigInt:
- "u64" => JsDefaultValue("0".to_string()),
- "u128" => JsDefaultValue("0".to_string()),
- "u256" => JsDefaultValue("0".to_string()),
+ "u64" => JsDefaultValue("BigInt(0)".to_string()),
+ "u128" => JsDefaultValue("BigInt(0)".to_string()),
+ "u256" => JsDefaultValue("BigInt(0)".to_string()),- Update the default value for
ByteArrayto useUint8Array:
- "ByteArray" => JsDefaultValue("\"\"".to_string()),
+ "ByteArray" => JsDefaultValue("new Uint8Array()".to_string()),- Consider adding a default value for custom types:
_ => JsDefaultValue(format!("new Custom{}()", value)),These changes will ensure consistency between the type mappings and default values.
| pub(crate) fn get_namespace_and_path(token: &Composite) -> (String, String, String) { | ||
| let ns_split = token.type_path.split("::").collect::<Vec<&str>>(); | ||
| if ns_split.len() < 2 { | ||
| panic!("type is invalid type_path has to be at least namespace::type"); | ||
| } | ||
| let ns = ns_split[0]; | ||
| let type_name = ns_split[ns_split.len() - 1]; | ||
| let namespace = ns.to_case(Case::Pascal); | ||
| (ns.to_owned(), namespace, type_name.to_owned()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using Result for error handling
Ohayo, sensei! The get_namespace_and_path function looks good overall, but it might benefit from using Result for error handling instead of panicking. This would allow the caller to handle the error gracefully.
-pub(crate) fn get_namespace_and_path(token: &Composite) -> (String, String, String) {
+pub(crate) fn get_namespace_and_path(token: &Composite) -> Result<(String, String, String), &'static str> {
let ns_split = token.type_path.split("::").collect::<Vec<&str>>();
if ns_split.len() < 2 {
- panic!("type is invalid type_path has to be at least namespace::type");
+ return Err("type is invalid: type_path has to be at least namespace::type");
}
let ns = ns_split[0];
let type_name = ns_split[ns_split.len() - 1];
let namespace = ns.to_case(Case::Pascal);
- (ns.to_owned(), namespace, type_name.to_owned())
+ Ok((ns.to_owned(), namespace, type_name.to_owned()))
}This change would allow callers to handle the error case without crashing the program.
📝 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.
| pub(crate) fn get_namespace_and_path(token: &Composite) -> (String, String, String) { | |
| let ns_split = token.type_path.split("::").collect::<Vec<&str>>(); | |
| if ns_split.len() < 2 { | |
| panic!("type is invalid type_path has to be at least namespace::type"); | |
| } | |
| let ns = ns_split[0]; | |
| let type_name = ns_split[ns_split.len() - 1]; | |
| let namespace = ns.to_case(Case::Pascal); | |
| (ns.to_owned(), namespace, type_name.to_owned()) | |
| } | |
| pub(crate) fn get_namespace_and_path(token: &Composite) -> Result<(String, String, String), &'static str> { | |
| let ns_split = token.type_path.split("::").collect::<Vec<&str>>(); | |
| if ns_split.len() < 2 { | |
| return Err("type is invalid: type_path has to be at least namespace::type"); | |
| } | |
| let ns = ns_split[0]; | |
| let type_name = ns_split[ns_split.len() - 1]; | |
| let namespace = ns.to_case(Case::Pascal); | |
| Ok((ns.to_owned(), namespace, type_name.to_owned())) | |
| } |
| const ERC_TORII_TPL: &str = "// Type definition for ERC__Balance struct | ||
| export type ERC__Type = 'ERC20' | 'ERC721'; | ||
| export interface ERC__Balance { | ||
| fieldOrder: string[]; |
There was a problem hiding this comment.
Inconsistent capitalization of 'fieldOrder'
Ohayo, sensei!
There is an inconsistency in the capitalization of fieldOrder between the interface definitions and their initializations. In the interfaces (lines 10~, 16~, 50~), it's fieldOrder, but in the initializations (lines 37~, 41~, 65~), it's fieldorder. This inconsistency may lead to runtime errors or unexpected behavior in TypeScript.
Here's a diff to fix the inconsistency:
-line 37~:
- fieldorder: ['balance', 'type', 'tokenmetadata'],
+ fieldOrder: ['balance', 'type', 'tokenMetadata'],
-line 41~:
- fieldorder: ['name', 'symbol', 'tokenId', 'decimals', 'contractAddress'],
+ fieldOrder: ['name', 'symbol', 'tokenId', 'decimals', 'contractAddress'],
-line 65~:
- fieldorder: ['name', 'symbol', 'tokenId', 'decimals', 'contractAddress'],
+ fieldOrder: ['name', 'symbol', 'tokenId', 'decimals', 'contractAddress'],Also applies to: 16-16, 50-50, 37-37, 41-41, 65-65
There was a problem hiding this comment.
Amazing for the first iteration on that @MartianGreed!
Some minor comments that will be good for subsequent PRs.
Also, for all the indentation with \t, spaces may be more common. Also, maybe we could generate something without caring that much about format, and part of integrating generated code, the commit may also include custom formatting.
It would also ease the reading in the code itself (in the rust code).
Let's merge and iterate!
| .replace("RecsType.", "").replace("Array", "[]") | ||
| // types should be lowercased | ||
| .to_lowercase(), | ||
| Token::Composite(t) => format!("models.{}", t.type_name()), |
There was a problem hiding this comment.
Are you sure at this level the composites tokens are only models?
You may end up with some enums that are simply types passed as inputs to systems?
Could be interesting to test with some enums and structs the are not models. Or models is a generic module where you have all the types?
| out | ||
| } | ||
|
|
||
| // Formats a system into a C# method used by the contract class |
There was a problem hiding this comment.
| // Formats a system into a C# method used by the contract class | |
| // Formats a system into a Typescript method used by the contract class |
In a next pass, could be helpful having an adjustment of comments to the very specific use-case here to contextualize more the reader.
|
|
||
| fn create_available_theme_enum_token() -> Composite { | ||
| Composite { | ||
| type_path: "core::test::AvailableTheme".to_owned(), |
There was a problem hiding this comment.
type_path: "core::test::AvailableTheme".to_owned(),
Those type of values that we have several time may be extracted in constants to ease changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
crates/dojo-bindgen/src/plugins/mod.rs (5)
12-12: New module looks promising, sensei!The addition of the
recsmodule aligns well with the PR objectives. It would be helpful to add a brief doc comment explaining the purpose of this module for future developers.Consider adding a doc comment like this:
/// Module for handling Record and Event Component System (RECS) functionality. pub mod recs;
36-73: Ohayo, sensei! The newBufferstruct is a masterpiece!The
Bufferstruct and its implementation provide a powerful and flexible way to manage collections of strings. The methods for creating, checking, pushing, inserting, and joining strings are well-designed and implemented. TheDerefandDerefMuttrait implementations allow for seamless use of the underlyingVec<String>.A few suggestions to consider:
- Add doc comments for the
Bufferstruct and its methods to explain their purpose and usage.- Consider adding error handling for the
insert_aftermethod, as it currently usesunwrap()which could panic if the position is not found.Overall, excellent work on this addition!
85-93: Ohayo, sensei! TheBindgenWritertrait is looking sharp!The
BindgenWritertrait provides a clean and standardized interface for writing generated code, which aligns perfectly with the PR's goal of enhancing modularity and extensibility. Great job!A couple of suggestions to make it even better:
- Consider adding a doc comment for the
get_pathmethod to explain its purpose.- You might want to make the
writemethod returnBindgenResult<()>instead ofBindgenResult<(PathBuf, Vec<u8>)>if the path and data are not needed after writing. This would simplify the interface.
95-104: TheBindgenModelGeneratortrait is looking good, sensei!This trait provides a clean interface for generating model code, which is great for modularity and extensibility. The use of
Compositetoken andBufferallows for flexible code generation.A few suggestions to enhance it further:
- Consider adding doc comments for the
generatemethod parameters to explain their purpose.- The TODO comment about adding a mutable reference to currently generated code is a good idea. You might want to create a GitHub issue to track this future improvement.
- The return type
BindgenResult<String>seems inconsistent with the use ofBuffer. Consider changing it toBindgenResult<()>if the generated code is being written directly to the buffer.
106-113: Ohayo, sensei! TheBindgenContractGeneratortrait is looking solid!This trait provides a well-structured interface for generating contract code, which is excellent for modularity and extensibility. The use of
DojoContract,Functiontoken, andBufferallows for comprehensive and flexible code generation.To make it even better:
- Consider adding doc comments for the
generatemethod and its parameters to explain their purpose and usage.- Similar to the
BindgenModelGeneratortrait, the return typeBindgenResult<String>seems inconsistent with the use ofBuffer. Consider changing it toBindgenResult<()>if the generated code is being written directly to the buffer.
📜 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 (1)
- crates/dojo-bindgen/src/plugins/mod.rs (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
crates/dojo-bindgen/src/plugins/mod.rs (3)
3-3: Ohayo, sensei! New imports look good!The added imports for
Deref,DerefMut,Composite, andFunctionare well-chosen for the new functionality. IncludingDojoContractin the existing import is also a nice touch for code organization.Also applies to: 7-7, 10-10
22-22: Enum update looks great, sensei!The addition of the
Recsvariant toBuiltinPluginsand itsfmt::Displayimplementation is consistent with the newrecsmodule.Ohayo! Don't forget to update any associated documentation or comments for the
BuiltinPluginsenum to include information about the newRecsvariant.Also applies to: 31-31
76-76: Sync-ing up theBuiltinPlugintrait, nice move sensei!Adding the
Syncmarker trait toBuiltinPluginis a great decision. This ensures that implementations can be safely shared across threads, which is crucial for concurrent processing. Good thinking on future-proofing the design!
Description
this is a draft I've left out code comments in code that will be removed afterward.
Revamping bindgen to make it more modular and more extandable.
We can discuss on
BindgenGeneratorwhich may evolve.I think this could be a great base to work. After finishing update for
--typescriptI will add a--recsto add retro compatibility.Tests
Added to documentation?
In code itself, as this is a very specific part of software. Might add a README to guide developper through creation of new plugins like godot for instance
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
TypescriptRecsPluginfor enhanced TypeScript code generation from Dojo models and contracts.Bug Fixes
TypescriptRecsPlugin.Documentation
Refactor
TypescriptPluginfor better code generation handling and maintainability.