feat(sozo): add full workspace and accept non-dojo targets #2207
Conversation
WalkthroughOhayo, sensei! The recent updates significantly enhance package management in the build and test processes, introducing optional package filtering for improved control. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BuildArgs
participant DojoCompiler
participant TestArgs
participant Metadata
User->>BuildArgs: Specify packages to build
BuildArgs->>DojoCompiler: Run build with specified packages
DojoCompiler-->>User: Log compilation process
User->>TestArgs: Specify packages to test
TestArgs->>Metadata: Retrieve package metadata
Metadata-->>User: Provide test results
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Files skipped from review as they are similar to previous changes (1)
Additional comments not posted (2)
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
Missing the package filter on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
crates/dojo-lang/src/utils.rs (2)
18-21: Consider removing commented-out debug lines or using a logging mechanism.The debug comments are currently commented out. It would be better to either remove them or use a logging mechanism to control verbosity.
- // Super verbose print, but useful to get the CfgSet. - // debug!(cfg_set = ?db.cfg_set(), crates = ?db.crates(), "Retrieving namespace configuration."); + tracing::debug!(cfg_set = ?db.cfg_set(), crates = ?db.crates(), "Retrieving namespace configuration.");
42-55: Consider removing commented-out debug lines or using a logging mechanism.The debug comments are currently commented out. It would be better to either remove them or use a logging mechanism to control verbosity.
- // Very verbose. - // tracing::debug!(config_path = %config_path.to_string_lossy(), "Reading Scarb.toml file for namespace config."); + tracing::debug!(config_path = %config_path.to_string_lossy(), "Reading Scarb.toml file for namespace config.");bin/sozo/src/commands/test.rs (1)
65-67: Incomplete implementation of thepackagesfield inTestArgsstruct.The
packagesfield is correctly declared and documented, but its usage is not fully implemented. There is a TODO comment indicating the need for further work. Please complete the implementation, possibly using the implementation inbuild.rsas a reference.
bin/sozo/src/commands/test.rs: Lines with TODO comment forpackagesfield.Analysis chain
New field
packagesinTestArgsstruct is well-implemented.The new field allows users to specify which packages to test. The documentation is clear and the implementation is consistent with the existing fields.
Ensure that the new field is utilized correctly in the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new `packages` field in the `TestArgs` struct. # Test: Search for the usage of the `packages` field. Expect: Correct usage in the codebase. rg --type rust -A 5 $'packages'Length of output: 11926
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (5)
crates/dojo-world/src/metadata.rs (5)
86-87: Ohayo, sensei! Add a brief doc comment for the new function.Adding a doc comment will improve readability and provide context for future developers.
/// Extracts Dojo metadata from a specified package.
95-96: Ohayo, sensei! Improve the error message for clarity.The error message can be more descriptive to help users understand the issue better.
- return Err(anyhow::anyhow!("[lib] package cannot have [[target.dojo]].")); + return Err(anyhow::anyhow!("Package with [lib] target cannot also have [[target.dojo]] target."));
121-122: Ohayo, sensei! Add a comment to explain the purpose ofmetadata_artifacts_load.Adding a comment here will help future developers understand why this function is called.
// Load additional artifacts metadata for the dojo.
128-129: Ohayo, sensei! Add a brief doc comment for the function.Adding a doc comment will improve readability and provide context for future developers.
/// Extracts Dojo metadata from the workspace.
137-154: Ohayo, sensei! Improve error handling for multiple dojo packages.The error message can be more descriptive to help users understand the issue better.
- Err(anyhow::anyhow!( - "Multiple packages with dojo target found in workspace. Please specify a package \ - using --package option or maybe one of them must be declared as a [lib]." - )), + Err(anyhow::anyhow!( + "Multiple packages with dojo target found in workspace. Please specify a package using the --package option or ensure only one package has the dojo target." + )),
| Ok(dojo_metadata) | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Consider logging the collected metadata at a higher log level.
Using info level instead of trace might be more appropriate for logging collected metadata.
- tracing::trace!(target: LOG_TARGET, ?dojo_metadata);
+ tracing::info!(target: LOG_TARGET, ?dojo_metadata, "Collected Dojo metadata from package.");Committable suggestion was skipped due to low confidence.
| let project_metadata = match package.manifest.metadata.dojo() { | ||
| Ok(m) => Ok(m), | ||
| Err(e) => { | ||
| if is_lib || !is_dojo { | ||
| Ok(ProjectMetadata::default()) | ||
| } else { | ||
| Err(anyhow::anyhow!( | ||
| "In manifest {} [dojo] package must have [[target.dojo]]: {}.", | ||
| ws.manifest_path(), | ||
| e | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Simplify the error handling logic.
The error handling logic can be simplified by using map_err to transform the error.
- let project_metadata = match package.manifest.metadata.dojo() {
- Ok(m) => Ok(m),
- Err(e) => {
- if is_lib || !is_dojo {
- Ok(ProjectMetadata::default())
- } else {
- Err(anyhow::anyhow!(
- "In manifest {} [dojo] package must have [[target.dojo]]: {}.",
- ws.manifest_path(),
- e
- ))
- }
- }
- }?;
+ let project_metadata = package.manifest.metadata.dojo().map_err(|e| {
+ if is_lib || !is_dojo {
+ anyhow::anyhow!("In manifest {} [dojo] package must have [[target.dojo]]: {}.", ws.manifest_path(), e)
+ } else {
+ ProjectMetadata::default()
+ }
+ })?;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 project_metadata = match package.manifest.metadata.dojo() { | |
| Ok(m) => Ok(m), | |
| Err(e) => { | |
| if is_lib || !is_dojo { | |
| Ok(ProjectMetadata::default()) | |
| } else { | |
| Err(anyhow::anyhow!( | |
| "In manifest {} [dojo] package must have [[target.dojo]]: {}.", | |
| ws.manifest_path(), | |
| e | |
| )) | |
| } | |
| } | |
| let project_metadata = package.manifest.metadata.dojo().map_err(|e| { | |
| if is_lib || !is_dojo { | |
| ProjectMetadata::default() | |
| } else { | |
| anyhow::anyhow!("In manifest {} [dojo] package must have [[target.dojo]]: {}.", ws.manifest_path(), e) | |
| } | |
| })?; |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2207 +/- ##
==========================================
- Coverage 69.06% 68.95% -0.12%
==========================================
Files 339 339
Lines 44052 44288 +236
==========================================
+ Hits 30426 30539 +113
- Misses 13626 13749 +123 ☔ View full report in Codecov by Sentry. |
| // TODO: if we want to output the manifests at the package level, | ||
| // we must iterate on the ws members, to find the location of the | ||
| // sole package with the `dojo` target. | ||
| // In this case, we can use this path to output the manifests. | ||
|
|
There was a problem hiding this comment.
Ohayo, sensei! Acknowledge the TODO comment.
The TODO comment indicates a potential future enhancement to output manifests at the package level.
Would you like me to create a GitHub issue to track this task, or provide assistance in implementing this feature?
| // Ensure `contracts` dir exist event if no contracts are compiled | ||
| // to avoid errors when loading manifests. | ||
| let base_contracts_dir = base_manifests_dir.join(CONTRACTS_DIR); | ||
| let base_contracts_abis_dir = base_abis_dir.join(CONTRACTS_DIR); | ||
| if !base_contracts_dir.exists() { | ||
| std::fs::create_dir_all(&base_contracts_dir)?; | ||
| } | ||
| if !base_contracts_abis_dir.exists() { | ||
| std::fs::create_dir_all(&base_contracts_abis_dir)?; | ||
| } | ||
|
|
There was a problem hiding this comment.
Ohayo, sensei! Nice addition of directory creation logic.
Ensuring the contracts directory exists even if no contracts are compiled prevents potential errors when loading manifests. Consider using fs::create_dir_all once for both directories.
- if !base_contracts_dir.exists() {
- std::fs::create_dir_all(&base_contracts_dir)?;
- }
- if !base_contracts_abis_dir.exists() {
- std::fs::create_dir_all(&base_contracts_abis_dir)?;
- }
+ [base_contracts_dir, base_contracts_abis_dir].iter().try_for_each(|dir| {
+ if !dir.exists() {
+ std::fs::create_dir_all(dir)
+ } else {
+ Ok(())
+ }
+ })?;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.
| // Ensure `contracts` dir exist event if no contracts are compiled | |
| // to avoid errors when loading manifests. | |
| let base_contracts_dir = base_manifests_dir.join(CONTRACTS_DIR); | |
| let base_contracts_abis_dir = base_abis_dir.join(CONTRACTS_DIR); | |
| if !base_contracts_dir.exists() { | |
| std::fs::create_dir_all(&base_contracts_dir)?; | |
| } | |
| if !base_contracts_abis_dir.exists() { | |
| std::fs::create_dir_all(&base_contracts_abis_dir)?; | |
| } | |
| // Ensure `contracts` dir exist event if no contracts are compiled | |
| // to avoid errors when loading manifests. | |
| let base_contracts_dir = base_manifests_dir.join(CONTRACTS_DIR); | |
| let base_contracts_abis_dir = base_abis_dir.join(CONTRACTS_DIR); | |
| [base_contracts_dir, base_contracts_abis_dir].iter().try_for_each(|dir| { | |
| if !dir.exists() { | |
| std::fs::create_dir_all(dir) | |
| } else { | |
| Ok(()) | |
| } | |
| })?; |
| // Ensure `models` dir exist event if no models are compiled | ||
| // to avoid errors when loading manifests. | ||
| let base_models_dir = base_manifests_dir.join(MODELS_DIR); | ||
| let base_models_abis_dir = base_abis_dir.join(MODELS_DIR); | ||
| if !base_models_dir.exists() { | ||
| std::fs::create_dir_all(&base_models_dir)?; | ||
| } | ||
| if !base_models_abis_dir.exists() { | ||
| std::fs::create_dir_all(&base_models_abis_dir)?; | ||
| } | ||
|
|
There was a problem hiding this comment.
Ohayo, sensei! Nice addition of directory creation logic.
Ensuring the models directory exists even if no models are compiled prevents potential errors when loading manifests. Consider using fs::create_dir_all once for both directories.
- if !base_models_dir.exists() {
- std::fs::create_dir_all(&base_models_dir)?;
- }
- if !base_models_abis_dir.exists() {
- std::fs::create_dir_all(&base_models_abis_dir)?;
- }
+ [base_models_dir, base_models_abis_dir].iter().try_for_each(|dir| {
+ if !dir.exists() {
+ std::fs::create_dir_all(dir)
+ } else {
+ Ok(())
+ }
+ })?;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.
| // Ensure `models` dir exist event if no models are compiled | |
| // to avoid errors when loading manifests. | |
| let base_models_dir = base_manifests_dir.join(MODELS_DIR); | |
| let base_models_abis_dir = base_abis_dir.join(MODELS_DIR); | |
| if !base_models_dir.exists() { | |
| std::fs::create_dir_all(&base_models_dir)?; | |
| } | |
| if !base_models_abis_dir.exists() { | |
| std::fs::create_dir_all(&base_models_abis_dir)?; | |
| } | |
| // Ensure `models` dir exist event if no models are compiled | |
| // to avoid errors when loading manifests. | |
| let base_models_dir = base_manifests_dir.join(MODELS_DIR); | |
| let base_models_abis_dir = base_abis_dir.join(MODELS_DIR); | |
| [base_models_dir, base_models_abis_dir].iter().try_for_each(|dir| { | |
| if !dir.exists() { | |
| std::fs::create_dir_all(dir) | |
| } else { | |
| Ok(()) | |
| } | |
| })?; |
Description
With this PR,
sozowill now accept again to build any Cairo code, even without the[[target.dojo]]. The dojo metadata (which contains the namespace configuration) is then a default configuration, as the namespace is not expected to be used in such situations.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Chores