feat(sozo): ensure sozo errors with dojo-core version mismatch #2364
Conversation
WalkthroughOhayo, sensei! The changes introduce a new function, Changes
Sequence Diagram(s)sequenceDiagram
participant Workspace
participant Package
participant BuildArgs
participant TestArgs
participant check_package_dojo_version
Workspace->>+BuildArgs: Iterates over packages
BuildArgs->>+check_package_dojo_version: Call to check version
check_package_dojo_version->>Package: Verify "dojo" version
Package-->>-check_package_dojo_version: Return version status
check_package_dojo_version-->>-BuildArgs: Return validation result
BuildArgs-->>-Workspace: Proceed with build
Workspace->>+TestArgs: Iterates over packages
TestArgs->>+check_package_dojo_version: Call to check version
check_package_dojo_version->>Package: Verify "dojo" version
Package-->>-check_package_dojo_version: Return version status
check_package_dojo_version-->>-TestArgs: Return validation result
TestArgs-->>-Workspace: Proceed with tests
Recent review detailsConfiguration used: .coderabbit.yaml Files ignored due to path filters (1)
Files selected for processing (2)
Files skipped from review as they are similar to previous changes (1)
Additional comments not posted (1)
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
|
glihm
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
bin/sozo/src/commands/mod.rs
Outdated
| pub fn check_package_dojo_version(ws: &Workspace<'_>, package: &Package) -> anyhow::Result<()> { | ||
| if let Some(dojo_dep) = | ||
| package.manifest.summary.dependencies.iter().find(|dep| dep.name.as_str() == "dojo") | ||
| { | ||
| let dojo_version = env!("CARGO_PKG_VERSION"); | ||
|
|
||
| if !dojo_dep.to_string().contains(dojo_version) { | ||
| if let Ok(cp) = ws.current_package() { | ||
| let path = | ||
| if cp.id == package.id { package.manifest_path() } else { ws.manifest_path() }; | ||
|
|
||
| anyhow::bail!( | ||
| "Found dojo-core version mismatch: expected {}. Please verify your dojo \ | ||
| dependency in {}", | ||
| dojo_version, | ||
| path | ||
| ) | ||
| } else { | ||
| // Virtual workspace. | ||
| anyhow::bail!( | ||
| "Found dojo-core version mismatch: expected {}. Please verify your dojo \ | ||
| dependency in {}", | ||
| dojo_version, | ||
| ws.manifest_path() | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Refactor for readability and enhance error message.
The function is well-structured but can be optimized by reducing the number of nested if statements. Additionally, the error message can be more descriptive.
Apply this diff to refactor the function and enhance the error message:
pub fn check_package_dojo_version(ws: &Workspace<'_>, package: &Package) -> anyhow::Result<()> {
if let Some(dojo_dep) = package.manifest.summary.dependencies.iter().find(|dep| dep.name.as_str() == "dojo") {
let dojo_version = env!("CARGO_PKG_VERSION");
if !dojo_dep.to_string().contains(dojo_version) {
let path = match ws.current_package() {
Ok(cp) if cp.id == package.id => package.manifest_path(),
_ => ws.manifest_path(),
};
anyhow::bail!(
"Found dojo-core version mismatch: expected version {}. Please verify your dojo dependency in {}",
dojo_version,
path
);
}
}
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.
| pub fn check_package_dojo_version(ws: &Workspace<'_>, package: &Package) -> anyhow::Result<()> { | |
| if let Some(dojo_dep) = | |
| package.manifest.summary.dependencies.iter().find(|dep| dep.name.as_str() == "dojo") | |
| { | |
| let dojo_version = env!("CARGO_PKG_VERSION"); | |
| if !dojo_dep.to_string().contains(dojo_version) { | |
| if let Ok(cp) = ws.current_package() { | |
| let path = | |
| if cp.id == package.id { package.manifest_path() } else { ws.manifest_path() }; | |
| anyhow::bail!( | |
| "Found dojo-core version mismatch: expected {}. Please verify your dojo \ | |
| dependency in {}", | |
| dojo_version, | |
| path | |
| ) | |
| } else { | |
| // Virtual workspace. | |
| anyhow::bail!( | |
| "Found dojo-core version mismatch: expected {}. Please verify your dojo \ | |
| dependency in {}", | |
| dojo_version, | |
| ws.manifest_path() | |
| ) | |
| } | |
| } | |
| } | |
| Ok(()) | |
| } | |
| pub fn check_package_dojo_version(ws: &Workspace<'_>, package: &Package) -> anyhow::Result<()> { | |
| if let Some(dojo_dep) = package.manifest.summary.dependencies.iter().find(|dep| dep.name.as_str() == "dojo") { | |
| let dojo_version = env!("CARGO_PKG_VERSION"); | |
| if !dojo_dep.to_string().contains(dojo_version) { | |
| let path = match ws.current_package() { | |
| Ok(cp) if cp.id == package.id => package.manifest_path(), | |
| _ => ws.manifest_path(), | |
| }; | |
| anyhow::bail!( | |
| "Found dojo-core version mismatch: expected version {}. Please verify your dojo dependency in {}", | |
| dojo_version, | |
| path | |
| ); | |
| } | |
| } | |
| Ok(()) | |
| } |
| for p in &packages { | ||
| check_package_dojo_version(&ws, p)?; | ||
| } |
There was a problem hiding this comment.
Add logging for better traceability.
The function call to check_package_dojo_version is correctly placed. However, adding a log statement to indicate the package being checked would improve traceability.
Apply this diff to add a log statement:
for p in &packages {
+ tracing::info!("Checking dojo version for package: {}", p.name);
check_package_dojo_version(&ws, p)?;
}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.
| for p in &packages { | |
| check_package_dojo_version(&ws, p)?; | |
| } | |
| for p in &packages { | |
| tracing::info!("Checking dojo version for package: {}", p.name); | |
| check_package_dojo_version(&ws, p)?; | |
| } |
| for p in &packages { | ||
| check_package_dojo_version(&ws, p)?; | ||
| } |
There was a problem hiding this comment.
Add logging for better traceability.
The function call to check_package_dojo_version is correctly placed. However, adding a log statement to indicate the package being checked would improve traceability.
Apply this diff to add a log statement:
for p in &packages {
+ tracing::info!("Checking dojo version for package: {}", p.name);
check_package_dojo_version(&ws, p)?;
}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.
| for p in &packages { | |
| check_package_dojo_version(&ws, p)?; | |
| } | |
| for p in &packages { | |
| tracing::info!("Checking dojo version for package: {}", p.name); | |
| check_package_dojo_version(&ws, p)?; | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2364 +/- ##
==========================================
- Coverage 67.49% 67.47% -0.03%
==========================================
Files 359 359
Lines 46971 47002 +31
==========================================
+ Hits 31704 31715 +11
- Misses 15267 15287 +20 ☔ View full report in Codecov by Sentry. |
Description
When the toolchain version changed (
dojoup -v 1.0.0-alpha.8), the dependency on dojo inside theScarb.tomlmust also change. To avoid weird error messages, sozo is now throwing an error if thedojo-coreversion mismatches the dependency version set by the user inScarb.toml.Summary by CodeRabbit
dojo_examplespackage from "1.0.0-alpha.4" to "1.0.0-alpha.8".