From b095968dff0f606eed8b319db033b92e41109766 Mon Sep 17 00:00:00 2001 From: Amaan Qureshi Date: Wed, 12 Nov 2025 01:26:10 -0500 Subject: [PATCH] refactor(cli): clean up version updating code This commit adds proper error types when updating the version across files --- Cargo.lock | 1 + crates/cli/Cargo.toml | 1 + crates/cli/src/main.rs | 2 +- crates/cli/src/version.rs | 364 ++++++++++++++++++++------------------ 4 files changed, 195 insertions(+), 173 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ce5d678b..5e95e3a6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2016,6 +2016,7 @@ dependencies = [ "similar", "streaming-iterator", "tempfile", + "thiserror 2.0.17", "tiny_http", "tree-sitter", "tree-sitter-config", diff --git a/crates/cli/Cargo.toml b/crates/cli/Cargo.toml index 31626eb2..c10b4652 100644 --- a/crates/cli/Cargo.toml +++ b/crates/cli/Cargo.toml @@ -60,6 +60,7 @@ serde.workspace = true serde_json.workspace = true similar.workspace = true streaming-iterator.workspace = true +thiserror.workspace = true tiny_http.workspace = true walkdir.workspace = true wasmparser.workspace = true diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 1c7fa957..3f83bf5f 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -1394,7 +1394,7 @@ impl Test { impl Version { fn run(self, current_dir: PathBuf) -> Result<()> { - version::Version::new(self.version, current_dir, self.bump).run() + Ok(version::Version::new(self.version, current_dir, self.bump).run()?) } } diff --git a/crates/cli/src/version.rs b/crates/cli/src/version.rs index e7601186..9406b58c 100644 --- a/crates/cli/src/version.rs +++ b/crates/cli/src/version.rs @@ -1,8 +1,7 @@ use std::{fs, path::PathBuf, process::Command}; -use anyhow::{anyhow, Context, Result}; use clap::ValueEnum; -use log::{error, info, warn}; +use log::{info, warn}; use regex::Regex; use semver::Version as SemverVersion; use std::cmp::Ordering; @@ -22,6 +21,36 @@ pub struct Version { pub bump: Option, } +#[derive(thiserror::Error, Debug)] +pub enum VersionError { + #[error(transparent)] + Json(#[from] serde_json::Error), + #[error(transparent)] + Io(#[from] std::io::Error), + #[error("Failed to update one or more files:\n\n{0}")] + Update(UpdateErrors), +} + +#[derive(thiserror::Error, Debug)] +pub struct UpdateErrors(Vec); + +impl std::fmt::Display for UpdateErrors { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + for error in &self.0 { + writeln!(f, "{error}\n")?; + } + Ok(()) + } +} + +#[derive(thiserror::Error, Debug)] +pub enum UpdateError { + #[error("Failed to update {1}:\n{0}")] + Io(std::io::Error, PathBuf), + #[error("Failed to run `{0}`:\n{1}")] + Command(&'static str, String), +} + impl Version { #[must_use] pub const fn new( @@ -36,7 +65,7 @@ impl Version { } } - pub fn run(mut self) -> Result<()> { + pub fn run(mut self) -> Result<(), VersionError> { let tree_sitter_json = self.current_dir.join("tree-sitter.json"); let tree_sitter_json = @@ -84,111 +113,100 @@ impl Version { let is_multigrammar = tree_sitter_json.grammars.len() > 1; - let mut has_failure = false; - let mut record_failures = |result: anyhow::Result<()>| { + let mut errors = Vec::new(); + + // Helper to push errors into the errors vector, returns true if an error was pushed + let mut push_err = |result: Result<(), UpdateError>| -> bool { if let Err(e) = result { - has_failure = true; - error!("{e}"); + errors.push(e); + return true; } + false }; - record_failures(self.update_treesitter_json().with_context(|| { - format!( - "Failed to update tree-sitter.json at {}", - self.current_dir.display() - ) - })); - record_failures(self.update_cargo_toml().with_context(|| { - format!( - "Failed to update Cargo.toml at {}", - self.current_dir.display() - ) - })); - record_failures(self.update_package_json().with_context(|| { - format!( - "Failed to update package.json at {}", - self.current_dir.display() - ) - })); - record_failures(self.update_makefile(is_multigrammar).with_context(|| { - format!( - "Failed to update Makefile at {}", - self.current_dir.display() - ) - })); - record_failures(self.update_cmakelists_txt().with_context(|| { - format!( - "Failed to update CMakeLists.txt at {}", - self.current_dir.display() - ) - })); - record_failures(self.update_pyproject_toml().with_context(|| { - format!( - "Failed to update pyproject.toml at {}", - self.current_dir.display() - ) - })); + push_err(self.update_treesitter_json()); - if has_failure { - // Return an empty error message to prevent double-reporting - Err(anyhow!("")) - } else { + // Only update Cargo.lock if Cargo.toml was updated + push_err(self.update_cargo_toml()).then(|| push_err(self.update_cargo_lock())); + + // Only update package-lock.json if package.json was updated + push_err(self.update_package_json()).then(|| push_err(self.update_package_lock_json())); + + push_err(self.update_makefile(is_multigrammar)); + push_err(self.update_cmakelists_txt()); + push_err(self.update_pyproject_toml()); + + if errors.is_empty() { Ok(()) + } else { + Err(VersionError::Update(UpdateErrors(errors))) } } - fn update_treesitter_json(&self) -> Result<()> { - let tree_sitter_json = &fs::read_to_string(self.current_dir.join("tree-sitter.json"))?; + fn update_file_with(&self, path: &PathBuf, update_fn: F) -> Result<(), UpdateError> + where + F: Fn(&str) -> String, + { + let content = fs::read_to_string(path).map_err(|e| UpdateError::Io(e, path.clone()))?; + let updated_content = update_fn(&content); + fs::write(path, updated_content).map_err(|e| UpdateError::Io(e, path.clone())) + } - let tree_sitter_json = tree_sitter_json - .lines() - .map(|line| { - if line.contains("\"version\":") { - let prefix_index = line.find("\"version\":").unwrap() + "\"version\":".len(); - let start_quote = line[prefix_index..].find('"').unwrap() + prefix_index + 1; - let end_quote = line[start_quote + 1..].find('"').unwrap() + start_quote + 1; + fn update_treesitter_json(&self) -> Result<(), UpdateError> { + let json_path = self.current_dir.join("tree-sitter.json"); + self.update_file_with(&json_path, |content| { + content + .lines() + .map(|line| { + if line.contains("\"version\":") { + let prefix_index = + line.find("\"version\":").unwrap() + "\"version\":".len(); + let start_quote = + line[prefix_index..].find('"').unwrap() + prefix_index + 1; + let end_quote = + line[start_quote + 1..].find('"').unwrap() + start_quote + 1; - format!( - "{}{}{}", - &line[..start_quote], - self.version.as_ref().unwrap(), - &line[end_quote..] - ) - } else { - line.to_string() - } - }) - .collect::>() - .join("\n") - + "\n"; + format!( + "{}{}{}", + &line[..start_quote], + self.version.as_ref().unwrap(), + &line[end_quote..] + ) + } else { + line.to_string() + } + }) + .collect::>() + .join("\n") + + "\n" + }) + } - fs::write(self.current_dir.join("tree-sitter.json"), tree_sitter_json)?; + fn update_cargo_toml(&self) -> Result<(), UpdateError> { + let cargo_toml_path = self.current_dir.join("Cargo.toml"); + if !cargo_toml_path.exists() { + return Ok(()); + } + + self.update_file_with(&cargo_toml_path, |content| { + content + .lines() + .map(|line| { + if line.starts_with("version =") { + format!("version = \"{}\"", self.version.as_ref().unwrap()) + } else { + line.to_string() + } + }) + .collect::>() + .join("\n") + + "\n" + })?; Ok(()) } - fn update_cargo_toml(&self) -> Result<()> { - if !self.current_dir.join("Cargo.toml").exists() { - return Ok(()); - } - - let cargo_toml = fs::read_to_string(self.current_dir.join("Cargo.toml"))?; - - let cargo_toml = cargo_toml - .lines() - .map(|line| { - if line.starts_with("version =") { - format!("version = \"{}\"", self.version.as_ref().unwrap()) - } else { - line.to_string() - } - }) - .collect::>() - .join("\n") - + "\n"; - - fs::write(self.current_dir.join("Cargo.toml"), cargo_toml)?; - + fn update_cargo_lock(&self) -> Result<(), UpdateError> { if self.current_dir.join("Cargo.lock").exists() { let Ok(cmd) = Command::new("cargo") .arg("generate-lockfile") @@ -201,8 +219,9 @@ impl Version { if !cmd.status.success() { let stderr = String::from_utf8_lossy(&cmd.stderr); - return Err(anyhow!( - "Failed to run `cargo generate-lockfile`:\n{stderr}" + return Err(UpdateError::Command( + "cargo generate-lockfile", + stderr.to_string(), )); } } @@ -210,37 +229,43 @@ impl Version { Ok(()) } - fn update_package_json(&self) -> Result<()> { - if !self.current_dir.join("package.json").exists() { + fn update_package_json(&self) -> Result<(), UpdateError> { + let package_json_path = self.current_dir.join("package.json"); + if !package_json_path.exists() { return Ok(()); } - let package_json = &fs::read_to_string(self.current_dir.join("package.json"))?; + self.update_file_with(&package_json_path, |content| { + content + .lines() + .map(|line| { + if line.contains("\"version\":") { + let prefix_index = + line.find("\"version\":").unwrap() + "\"version\":".len(); + let start_quote = + line[prefix_index..].find('"').unwrap() + prefix_index + 1; + let end_quote = + line[start_quote + 1..].find('"').unwrap() + start_quote + 1; - let package_json = package_json - .lines() - .map(|line| { - if line.contains("\"version\":") { - let prefix_index = line.find("\"version\":").unwrap() + "\"version\":".len(); - let start_quote = line[prefix_index..].find('"').unwrap() + prefix_index + 1; - let end_quote = line[start_quote + 1..].find('"').unwrap() + start_quote + 1; + format!( + "{}{}{}", + &line[..start_quote], + self.version.as_ref().unwrap(), + &line[end_quote..] + ) + } else { + line.to_string() + } + }) + .collect::>() + .join("\n") + + "\n" + })?; - format!( - "{}{}{}", - &line[..start_quote], - self.version.as_ref().unwrap(), - &line[end_quote..] - ) - } else { - line.to_string() - } - }) - .collect::>() - .join("\n") - + "\n"; - - fs::write(self.current_dir.join("package.json"), package_json)?; + Ok(()) + } + fn update_package_lock_json(&self) -> Result<(), UpdateError> { if self.current_dir.join("package-lock.json").exists() { let Ok(cmd) = Command::new("npm") .arg("install") @@ -253,82 +278,77 @@ impl Version { if !cmd.status.success() { let stderr = String::from_utf8_lossy(&cmd.stderr); - return Err(anyhow!("Failed to run `npm install`:\n{stderr}")); + return Err(UpdateError::Command("npm install", stderr.to_string())); } } Ok(()) } - fn update_makefile(&self, is_multigrammar: bool) -> Result<()> { - let makefile = if is_multigrammar { - if !self.current_dir.join("common").join("common.mak").exists() { - return Ok(()); - } - - fs::read_to_string(self.current_dir.join("Makefile"))? + fn update_makefile(&self, is_multigrammar: bool) -> Result<(), UpdateError> { + let makefile_path = if is_multigrammar { + self.current_dir.join("common").join("common.mak") } else { - if !self.current_dir.join("Makefile").exists() { - return Ok(()); - } - - fs::read_to_string(self.current_dir.join("Makefile"))? + self.current_dir.join("Makefile") }; - let makefile = makefile - .lines() - .map(|line| { - if line.starts_with("VERSION") { - format!("VERSION := {}", self.version.as_ref().unwrap()) - } else { - line.to_string() - } - }) - .collect::>() - .join("\n") - + "\n"; - - fs::write(self.current_dir.join("Makefile"), makefile)?; + self.update_file_with(&makefile_path, |content| { + content + .lines() + .map(|line| { + if line.starts_with("VERSION") { + format!("VERSION := {}", self.version.as_ref().unwrap()) + } else { + line.to_string() + } + }) + .collect::>() + .join("\n") + + "\n" + })?; Ok(()) } - fn update_cmakelists_txt(&self) -> Result<()> { - if !self.current_dir.join("CMakeLists.txt").exists() { + fn update_cmakelists_txt(&self) -> Result<(), UpdateError> { + let cmake_lists_path = self.current_dir.join("CMakeLists.txt"); + if !cmake_lists_path.exists() { return Ok(()); } - let cmake = fs::read_to_string(self.current_dir.join("CMakeLists.txt"))?; - - let re = Regex::new(r#"(\s*VERSION\s+)"[0-9]+\.[0-9]+\.[0-9]+""#)?; - let cmake = re.replace(&cmake, format!(r#"$1"{}""#, self.version.as_ref().unwrap())); - - fs::write(self.current_dir.join("CMakeLists.txt"), cmake.as_bytes())?; + self.update_file_with(&cmake_lists_path, |content| { + let re = Regex::new(r#"(\s*VERSION\s+)"[0-9]+\.[0-9]+\.[0-9]+""#) + .expect("Failed to compile regex"); + re.replace( + content, + format!(r#"$1"{}""#, self.version.as_ref().unwrap()), + ) + .to_string() + })?; Ok(()) } - fn update_pyproject_toml(&self) -> Result<()> { - if !self.current_dir.join("pyproject.toml").exists() { + fn update_pyproject_toml(&self) -> Result<(), UpdateError> { + let pyproject_toml_path = self.current_dir.join("pyproject.toml"); + if !pyproject_toml_path.exists() { return Ok(()); } - let pyproject_toml = fs::read_to_string(self.current_dir.join("pyproject.toml"))?; - - let pyproject_toml = pyproject_toml - .lines() - .map(|line| { - if line.starts_with("version =") { - format!("version = \"{}\"", self.version.as_ref().unwrap()) - } else { - line.to_string() - } - }) - .collect::>() - .join("\n") - + "\n"; - - fs::write(self.current_dir.join("pyproject.toml"), pyproject_toml)?; + self.update_file_with(&pyproject_toml_path, |content| { + content + .lines() + .map(|line| { + if line.starts_with("version =") { + format!("version = \"{}\"", self.version.as_ref().unwrap()) + } else { + line.to_string() + } + }) + .collect::>() + .join("\n") + + "\n" + })?; Ok(()) }