Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sozo): apply semver to tag versions #2909

Merged
merged 1 commit into from
Jan 14, 2025
Merged

feat(sozo): apply semver to tag versions #2909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 41 additions & 18 deletions bin/sozo/src/commands/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use clap::Subcommand;
use events::EventsArgs;
use scarb::core::{Config, Package, Workspace};
use semver::{Version, VersionReq};
use tracing::info_span;

pub(crate) mod auth;
Expand Down Expand Up @@ -132,30 +133,52 @@

let dojo_dep_str = dojo_dep.to_string();

dbg!(&dojo_dep_str);
dbg!(&dojo_version);
Comment on lines +136 to +137
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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);


Check warning on line 138 in bin/sozo/src/commands/mod.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/mod.rs#L136-L138 

Added lines #L136 - L138 were not covered by tests
// Only in case of git dependency with an explicit tag, we check if the tag is the same as
// the current version.
if dojo_dep_str.contains("git+")
&& dojo_dep_str.contains("tag=v")
&& !dojo_dep_str.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()
)
// safe to unwrap since we know the string contains "tag=v".
// "dojo * (git+https://github.com/dojoengine/dojo?tag=v1.0.10)"
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]);
Comment on lines +154 to +155
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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_req_version = VersionReq::parse(&format!(">={}", major_minor)).unwrap();
Comment on lines +147 to +156
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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))?;


if !dojo_req_version.matches(&dojo_dep_version) {
if let Ok(cp) = ws.current_package() {

Check warning on line 159 in bin/sozo/src/commands/mod.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/mod.rs#L147-L159 

Added lines #L147 - L159 were not covered by tests
// Selected package.
let path = if cp.id == package.id {
package.manifest_path()

Check warning on line 162 in bin/sozo/src/commands/mod.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/mod.rs#L161-L162 

Added lines #L161 - L162 were not covered by tests
} else {
ws.manifest_path()

Check warning on line 164 in bin/sozo/src/commands/mod.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/mod.rs#L164 

Added line #L164 was not covered by tests
};

anyhow::bail!(
"Found dojo-core version mismatch: expected {}. Please verify your dojo \
dependency in {}",
dojo_req_version,
path
)

Check warning on line 172 in bin/sozo/src/commands/mod.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/mod.rs#L167-L172 

Added lines #L167 - L172 were not covered by tests
} else {
// Virtual workspace.
anyhow::bail!(
"Found dojo-core version mismatch: expected {}. Please verify your dojo \
dependency in {}",
dojo_req_version,
ws.manifest_path()
)

Check warning on line 180 in bin/sozo/src/commands/mod.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/mod.rs#L175-L180 

Added lines #L175 - L180 were not covered by tests
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/dojo/core-cairo-test/Scarb.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ version = 1

[[package]]
name = "dojo"
version = "1.0.0-rc.0"
version = "1.0.10"
dependencies = [
"dojo_plugin",
]
Expand Down
Loading