feat(sozo): apply semver to tag versions #2909
Conversation
WalkthroughOhayo, sensei! The changes focus on enhancing the version compatibility checking mechanism for Dojo packages. The modification introduces a more sophisticated approach to comparing Dojo dependency versions by leveraging the Changes
Sequence DiagramsequenceDiagram
participant Package as Current Package
participant DepChecker as Dependency Version Checker
participant SemVer as Semver Parser
Package->>DepChecker: Request version check
DepChecker->>SemVer: Extract dependency version
SemVer-->>DepChecker: Parsed Version
DepChecker->>DepChecker: Create Version Requirement
DepChecker->>SemVer: Check Version Compatibility
SemVer-->>DepChecker: Compatibility Result
DepChecker-->>Package: Return Compatibility Status
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
bin/sozo/src/commands/mod.rs (1)
147-151: Ohayo sensei! Consider using a more robust method to extract the tag version.Splitting
dojo_dep_strbased on hardcoded substrings like"tag=v"can be brittle. If the dependency format changes, this method may fail. Consider using URL parsing libraries or regular expressions to extract the tag parameter more reliably.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/dojo/core-cairo-test/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
bin/sozo/src/commands/mod.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (1)
bin/sozo/src/commands/mod.rs (1)
145-181: Ohayo sensei! Great job enhancing version compatibility checking usingsemver.The introduction of semantic versioning improves the reliability and flexibility of dependency management within the Dojo project.
| dbg!(&dojo_dep_str); | ||
| dbg!(&dojo_version); |
There was a problem hiding this comment.
Ohayo sensei! Please remove debug statements before committing code.
The use of dbg!() macro is intended for debugging purposes and should not be included in production code as it can clutter output and expose internal state.
Apply this diff to remove the debug statements:
- dbg!(&dojo_dep_str);
- dbg!(&dojo_version);| let version_parts: Vec<&str> = dojo_version.split('.').collect(); | ||
| let major_minor = format!("{}.{}", version_parts[0], version_parts[1]); |
There was a problem hiding this comment.
Ohayo sensei! Ensure dojo_version has major and minor components to prevent panics.
Accessing version_parts[0] and version_parts[1] without verifying the length may cause an index out of bounds panic if dojo_version doesn't contain both major and minor parts.
Modify the code to check the length before accessing the elements:
let version_parts: Vec<&str> = dojo_version.split('.').collect();
+ if version_parts.len() < 2 {
+ anyhow::bail!("Invalid dojo version format: {}", dojo_version);
+ }
let major_minor = format!("{}.{}", version_parts[0], version_parts[1]);📝 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 version_parts: Vec<&str> = dojo_version.split('.').collect(); | |
| let major_minor = format!("{}.{}", version_parts[0], version_parts[1]); | |
| let version_parts: Vec<&str> = dojo_version.split('.').collect(); | |
| if version_parts.len() < 2 { | |
| anyhow::bail!("Invalid dojo version format: {}", dojo_version); | |
| } | |
| let major_minor = format!("{}.{}", version_parts[0], version_parts[1]); |
| let dojo_dep_version = dojo_dep_str.split("tag=v") | ||
| .nth(1) // Get the part after "tag=v" | ||
| .map(|s| s.trim_end_matches(')')) | ||
| .expect("Unexpected dojo dependency format"); | ||
|
|
||
| let dojo_dep_version = Version::parse(dojo_dep_version).unwrap(); | ||
|
|
||
| let version_parts: Vec<&str> = dojo_version.split('.').collect(); | ||
| let major_minor = format!("{}.{}", version_parts[0], version_parts[1]); | ||
| let dojo_req_version = VersionReq::parse(&format!(">={}", major_minor)).unwrap(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Handle potential parsing errors to prevent panics.
Using .unwrap() when parsing versions can lead to runtime panics if the input strings are not in the expected format. To enhance robustness, handle these errors gracefully using map_err or the ? operator.
Apply this diff to handle parsing errors:
- let dojo_dep_version = Version::parse(dojo_dep_version).unwrap();
+ let dojo_dep_version = Version::parse(dojo_dep_version)
+ .map_err(|e| anyhow::anyhow!("Failed to parse dojo dependency version: {}", e))?;
...
- let dojo_req_version = VersionReq::parse(&format!(">={}", major_minor)).unwrap();
+ let dojo_req_version = VersionReq::parse(&format!(">={}", major_minor))
+ .map_err(|e| anyhow::anyhow!("Failed to parse required dojo version: {}", e))?;📝 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 dojo_dep_version = dojo_dep_str.split("tag=v") | |
| .nth(1) // Get the part after "tag=v" | |
| .map(|s| s.trim_end_matches(')')) | |
| .expect("Unexpected dojo dependency format"); | |
| let dojo_dep_version = Version::parse(dojo_dep_version).unwrap(); | |
| let version_parts: Vec<&str> = dojo_version.split('.').collect(); | |
| let major_minor = format!("{}.{}", version_parts[0], version_parts[1]); | |
| let dojo_req_version = VersionReq::parse(&format!(">={}", major_minor)).unwrap(); | |
| let dojo_dep_version = dojo_dep_str.split("tag=v") | |
| .nth(1) // Get the part after "tag=v" | |
| .map(|s| s.trim_end_matches(')')) | |
| .expect("Unexpected dojo dependency format"); | |
| let dojo_dep_version = Version::parse(dojo_dep_version) | |
| .map_err(|e| anyhow::anyhow!("Failed to parse dojo dependency version: {}", e))?; | |
| let version_parts: Vec<&str> = dojo_version.split('.').collect(); | |
| let major_minor = format!("{}.{}", version_parts[0], version_parts[1]); | |
| let dojo_req_version = VersionReq::parse(&format!(">={}", major_minor)) | |
| .map_err(|e| anyhow::anyhow!("Failed to parse required dojo version: {}", e))?; |
glihm
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2909 +/- ##
==========================================
- Coverage 56.05% 56.04% -0.02%
==========================================
Files 449 449
Lines 57927 57944 +17
==========================================
- Hits 32473 32472 -1
- Misses 25454 25472 +18 ☔ View full report in Codecov by Sentry. |
This PR adds semver behavior for the usage of dojo versions, since we can't use
scarbs.xyzyet.Summary by CodeRabbit
New Features
Refactor