From 4525a504b3964c675a5b927661ef57001ba9eaa8 Mon Sep 17 00:00:00 2001 From: ya7010 Date: Thu, 6 Aug 2026 14:12:27 +0900 Subject: [PATCH 1/5] fix: improve config load warnings --- Cargo.lock | 12 +++ Cargo.toml | 1 + crates/tombi-config/src/error.rs | 14 ++- rust/serde_tombi/Cargo.toml | 1 + rust/serde_tombi/src/config.rs | 166 +++++++++++++++++++++++-------- rust/serde_tombi/src/de.rs | 2 +- rust/serde_tombi/src/de/error.rs | 14 ++- 7 files changed, 160 insertions(+), 50 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 09b9f2942..1923ffaf5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2250,6 +2250,17 @@ dependencies = [ "serde", ] +[[package]] +name = "serde_path_to_error" +version = "0.1.20" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "10a9ff822e371bb5403e391ecd83e182e0e77ba7f6fe0160b795797109d1b457" +dependencies = [ + "itoa", + "serde", + "serde_core", +] + [[package]] name = "serde_repr" version = "0.1.20" @@ -2271,6 +2282,7 @@ dependencies = [ "log", "pretty_assertions", "serde", + "serde_path_to_error", "serde_tombi_macros", "tempfile", "textwrap", diff --git a/Cargo.toml b/Cargo.toml index e8762b809..984527870 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -67,6 +67,7 @@ schemars = { version = "1.2.1", features = ["preserve_order", "url2"] } semver = "1.0.17" serde = { version = "1.0.219", features = ["derive"] } serde_json = { version = "1.0.140", features = ["preserve_order"] } +serde_path_to_error = "0.1.20" serde_tombi = { path = "rust/serde_tombi", default-features = false } serde_tombi_macros = { path = "rust/serde_tombi_macros" } similar = { version = "2.7.0" } diff --git a/crates/tombi-config/src/error.rs b/crates/tombi-config/src/error.rs index 8ef57fe31..f47728e85 100644 --- a/crates/tombi-config/src/error.rs +++ b/crates/tombi-config/src/error.rs @@ -9,11 +9,17 @@ pub enum Error { #[error("config file not found: {config_path:?}")] ConfigFileNotFound { config_path: std::path::PathBuf }, - #[error("failed to read {config_path:?}")] - ConfigFileReadFailed { config_path: std::path::PathBuf }, + #[error("failed to read {config_path:?}: {reason}")] + ConfigFileReadFailed { + config_path: std::path::PathBuf, + reason: String, + }, - #[error("failed to parse {config_path:?}")] - ConfigFileParseFailed { config_path: std::path::PathBuf }, + #[error("failed to parse {config_path:?}: {reason}")] + ConfigFileParseFailed { + config_path: std::path::PathBuf, + reason: String, + }, #[error("unsupported config file: {config_path:?}")] ConfigFileUnsupported { config_path: std::path::PathBuf }, diff --git a/rust/serde_tombi/Cargo.toml b/rust/serde_tombi/Cargo.toml index ccadf7068..b10f9130c 100644 --- a/rust/serde_tombi/Cargo.toml +++ b/rust/serde_tombi/Cargo.toml @@ -16,6 +16,7 @@ dirs.workspace = true itertools.workspace = true log.workspace = true serde.workspace = true +serde_path_to_error.workspace = true serde_tombi_macros.workspace = true textwrap.workspace = true thiserror.workspace = true diff --git a/rust/serde_tombi/src/config.rs b/rust/serde_tombi/src/config.rs index f323e4f36..826579908 100644 --- a/rust/serde_tombi/src/config.rs +++ b/rust/serde_tombi/src/config.rs @@ -58,6 +58,31 @@ struct Tool { tombi: Option, } +#[inline] +fn config_file_parse_warning_message( + config_path: &std::path::Path, + error: &crate::de::Error, +) -> String { + if error.is_value_error() { + format!("Failed to parse config file {config_path:?} — {error}") + } else { + format!("Failed to parse config file {config_path:?}") + } +} + +#[inline] +fn config_file_parse_error( + config_path: &std::path::Path, + error: crate::de::Error, +) -> tombi_config::Error { + log::warn!("{}", config_file_parse_warning_message(config_path, &error)); + + tombi_config::Error::ConfigFileParseFailed { + config_path: config_path.to_owned(), + reason: error.to_string(), + } +} + pub fn try_from_path>( config_path: P, ) -> Result, tombi_config::Error> { @@ -69,27 +94,24 @@ pub fn try_from_path>( }); } - let Ok(config_text) = std::fs::read_to_string(config_path) else { - return Err(tombi_config::Error::ConfigFileReadFailed { + let config_text = std::fs::read_to_string(config_path).map_err(|error| { + log::warn!("Failed to read config file {config_path:?}"); + + tombi_config::Error::ConfigFileReadFailed { config_path: config_path.to_owned(), - }); - }; + reason: error.to_string(), + } + })?; match config_path.file_name().and_then(|name| name.to_str()) { Some(DOT_TOMBI_TOML_FILENAME | TOMBI_TOML_FILENAME | CONFIG_TOML_FILENAME) => { - match crate::config::from_str(&config_text, config_path) { - Ok(tombi_config) => Ok(Some(tombi_config)), - Err(_) => Err(tombi_config::Error::ConfigFileParseFailed { - config_path: config_path.to_owned(), - }), - } + crate::config::from_str(&config_text, config_path) + .map(Some) + .map_err(|error| config_file_parse_error(config_path, error)) } Some(PYPROJECT_TOML_FILENAME) => { - let Ok(pyproject_toml) = PyProjectToml::from_str(&config_text, config_path) else { - return Err(tombi_config::Error::ConfigFileParseFailed { - config_path: config_path.to_owned(), - }); - }; + let pyproject_toml = PyProjectToml::from_str(&config_text, config_path) + .map_err(|error| config_file_parse_error(config_path, error))?; if let Some(Tool { tombi: Some(tombi_config), }) = pyproject_toml.tool @@ -119,14 +141,18 @@ pub fn load_with_path_and_level( if config_path.is_file() { log::debug!("Project config found at {:?}", config_path); - let Some(config) = try_from_path(&config_path)? else { - unreachable!( - "project config should always be parsed successfully: {:?}", - config_path - ); - }; - - return Ok((config, Some(config_path), ConfigLevel::Project)); + match try_from_path(&config_path) { + Ok(Some(config)) => { + return Ok((config, Some(config_path), ConfigLevel::Project)); + } + Ok(None) => { + unreachable!( + "project config should always be parsed successfully: {:?}", + config_path + ); + } + Err(_) => {} + } } } @@ -139,20 +165,13 @@ pub fn load_with_path_and_level( pyproject_toml_path ); - match try_from_path(&pyproject_toml_path) { - Ok(Some(config)) => { + match try_from_path(&pyproject_toml_path).ok().flatten() { + Some(config) => { return Ok((config, Some(pyproject_toml_path), ConfigLevel::Project)); } - Ok(None) => { + None => { log::debug!("No [tool.tombi] found in {:?}", pyproject_toml_path); } - Err(error) => { - log::debug!( - "Failed to parse pyproject.toml file for config at {:?}: {}", - pyproject_toml_path, - error - ); - } }; } @@ -165,15 +184,19 @@ pub fn load_with_path_and_level( if let Some((user_config_path, config_level)) = get_user_or_system_tombi_config_path_and_level() { log::debug!("{CONFIG_TOML_FILENAME} found at {:?}", user_config_path); - let Some(config) = try_from_path(&user_config_path)? else { - unreachable!("{CONFIG_TOML_FILENAME} should always be parsed successfully."); - }; - Ok((config, Some(user_config_path), config_level)) - } else { - log::debug!("config file not found, use default config"); - - Ok((Config::default(), None, ConfigLevel::Default)) + match try_from_path(&user_config_path).ok().flatten() { + Some(config) => { + return Ok((config, Some(user_config_path), config_level)); + } + None => { + unreachable!("{CONFIG_TOML_FILENAME} should always be parsed successfully."); + } + } } + + log::debug!("config file not found, use default config"); + + Ok((Config::default(), None, ConfigLevel::Default)) } #[inline] @@ -265,6 +288,67 @@ mod tests { .unwrap() } + #[test] + fn reports_config_parse_warnings() { + let temp_dir = temp_test_dir("config-parse-warnings"); + + for (config_filename, config_text, error_detail) in [ + (TOMBI_TOML_FILENAME, "toml-version = ", None), + ( + TOMBI_TOML_FILENAME, + "[format.rules]\nindent-width = \"two\"\n", + Some("format.rules.indent-width: invalid type: string \"two\", expected u8"), + ), + ( + PYPROJECT_TOML_FILENAME, + "[tool.tombi.format.rules]\nindent-width = \"two\"\n", + Some( + "tool.tombi.format.rules.indent-width: invalid type: string \"two\", expected u8", + ), + ), + ] { + let config_path = temp_dir.path().join(config_filename); + let error = if config_filename == PYPROJECT_TOML_FILENAME { + PyProjectToml::from_str(config_text, &config_path).unwrap_err() + } else { + crate::config::from_str(config_text, &config_path).unwrap_err() + }; + let message = config_file_parse_warning_message(&config_path, &error); + + assert_eq!( + message, + match error_detail { + Some(error_detail) => + format!("Failed to parse config file {config_path:?} — {error_detail}"), + None => format!("Failed to parse config file {config_path:?}"), + } + ); + } + } + + #[test] + fn loads_next_project_config_after_parse_error() { + let temp_dir = temp_test_dir("config-parse-error-fallback"); + let nested_dir = temp_dir.path().join("workspace/nested"); + + std::fs::create_dir_all(&nested_dir).unwrap(); + write_file( + &temp_dir.path().join(DOT_TOMBI_TOML_FILENAME), + "toml-version = ", + ); + write_file( + &temp_dir.path().join(TOMBI_TOML_FILENAME), + "toml-version = \"v1.0.0\"\n", + ); + + let (config, config_path, config_level) = + load_with_path_and_level(Some(nested_dir)).unwrap(); + + assert_eq!(config.toml_version, Some(TomlVersion::V1_0_0)); + assert_eq!(config_path, Some(temp_dir.path().join(TOMBI_TOML_FILENAME))); + assert_eq!(config_level, ConfigLevel::Project); + } + #[test] fn loads_project_config_from_dot_config_tombi_toml_before_pyproject() { let temp_dir = temp_test_dir("dot-config-tombi"); diff --git a/rust/serde_tombi/src/de.rs b/rust/serde_tombi/src/de.rs index a0168480c..3cf4d0fb2 100644 --- a/rust/serde_tombi/src/de.rs +++ b/rust/serde_tombi/src/de.rs @@ -109,7 +109,7 @@ impl Deserializer<'_> { where T: DeserializeOwned, { - Ok(T::deserialize(&document)?) + Ok(serde_path_to_error::deserialize(&document)?) } async fn get_toml_version( diff --git a/rust/serde_tombi/src/de/error.rs b/rust/serde_tombi/src/de/error.rs index 20e2cb570..75738a5ef 100644 --- a/rust/serde_tombi/src/de/error.rs +++ b/rust/serde_tombi/src/de/error.rs @@ -4,6 +4,12 @@ use itertools::Itertools; #[error(transparent)] pub struct Error(Box); +impl Error { + pub(crate) fn is_value_error(&self) -> bool { + matches!(self.0.as_ref(), InnerError::DocumentDeserializeWithPath(_)) + } +} + #[derive(Debug, thiserror::Error)] enum InnerError { #[error(transparent)] @@ -16,7 +22,7 @@ enum InnerError { SchemaStore(#[from] tombi_schema_store::Error), #[error(transparent)] - DocumentDeserialize(#[from] tombi_document::de::Error), + DocumentDeserializeWithPath(#[from] serde_path_to_error::Error), #[error("{}", .0.iter().map(|e| e.to_string()).collect_vec().join(", "))] Parser(Vec), @@ -61,8 +67,8 @@ impl From for Error { } } -impl From for Error { - fn from(error: tombi_document::de::Error) -> Self { - Self(Box::new(InnerError::DocumentDeserialize(error))) +impl From> for Error { + fn from(error: serde_path_to_error::Error) -> Self { + Self(Box::new(InnerError::DocumentDeserializeWithPath(error))) } } From f3a28a5468ccf041ba71ed95d5f3db5e24af3a89 Mon Sep 17 00:00:00 2001 From: ya7010 Date: Thu, 6 Aug 2026 14:26:14 +0900 Subject: [PATCH 2/5] refactor: simplify config file parse error handling and remove unused warning message function --- rust/serde_tombi/src/config.rs | 52 +------------------------------- rust/serde_tombi/src/de/error.rs | 6 ---- 2 files changed, 1 insertion(+), 57 deletions(-) diff --git a/rust/serde_tombi/src/config.rs b/rust/serde_tombi/src/config.rs index 826579908..8c17be47a 100644 --- a/rust/serde_tombi/src/config.rs +++ b/rust/serde_tombi/src/config.rs @@ -58,24 +58,12 @@ struct Tool { tombi: Option, } -#[inline] -fn config_file_parse_warning_message( - config_path: &std::path::Path, - error: &crate::de::Error, -) -> String { - if error.is_value_error() { - format!("Failed to parse config file {config_path:?} — {error}") - } else { - format!("Failed to parse config file {config_path:?}") - } -} - #[inline] fn config_file_parse_error( config_path: &std::path::Path, error: crate::de::Error, ) -> tombi_config::Error { - log::warn!("{}", config_file_parse_warning_message(config_path, &error)); + log::warn!("{}", format!("Failed to parse config file {config_path:?}")); tombi_config::Error::ConfigFileParseFailed { config_path: config_path.to_owned(), @@ -288,44 +276,6 @@ mod tests { .unwrap() } - #[test] - fn reports_config_parse_warnings() { - let temp_dir = temp_test_dir("config-parse-warnings"); - - for (config_filename, config_text, error_detail) in [ - (TOMBI_TOML_FILENAME, "toml-version = ", None), - ( - TOMBI_TOML_FILENAME, - "[format.rules]\nindent-width = \"two\"\n", - Some("format.rules.indent-width: invalid type: string \"two\", expected u8"), - ), - ( - PYPROJECT_TOML_FILENAME, - "[tool.tombi.format.rules]\nindent-width = \"two\"\n", - Some( - "tool.tombi.format.rules.indent-width: invalid type: string \"two\", expected u8", - ), - ), - ] { - let config_path = temp_dir.path().join(config_filename); - let error = if config_filename == PYPROJECT_TOML_FILENAME { - PyProjectToml::from_str(config_text, &config_path).unwrap_err() - } else { - crate::config::from_str(config_text, &config_path).unwrap_err() - }; - let message = config_file_parse_warning_message(&config_path, &error); - - assert_eq!( - message, - match error_detail { - Some(error_detail) => - format!("Failed to parse config file {config_path:?} — {error_detail}"), - None => format!("Failed to parse config file {config_path:?}"), - } - ); - } - } - #[test] fn loads_next_project_config_after_parse_error() { let temp_dir = temp_test_dir("config-parse-error-fallback"); diff --git a/rust/serde_tombi/src/de/error.rs b/rust/serde_tombi/src/de/error.rs index 75738a5ef..6a71fb4ea 100644 --- a/rust/serde_tombi/src/de/error.rs +++ b/rust/serde_tombi/src/de/error.rs @@ -4,12 +4,6 @@ use itertools::Itertools; #[error(transparent)] pub struct Error(Box); -impl Error { - pub(crate) fn is_value_error(&self) -> bool { - matches!(self.0.as_ref(), InnerError::DocumentDeserializeWithPath(_)) - } -} - #[derive(Debug, thiserror::Error)] enum InnerError { #[error(transparent)] From 24e59cde80ac94c8bd01fefcb2e923f0571a35b4 Mon Sep 17 00:00:00 2001 From: ya7010 Date: Thu, 6 Aug 2026 14:28:04 +0900 Subject: [PATCH 3/5] refactor: remove serde_path_to_error dependency and simplify error handling in deserialization --- Cargo.lock | 12 ------------ Cargo.toml | 1 - rust/serde_tombi/Cargo.toml | 1 - rust/serde_tombi/src/de.rs | 2 +- rust/serde_tombi/src/de/error.rs | 8 ++++---- 5 files changed, 5 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1923ffaf5..09b9f2942 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2250,17 +2250,6 @@ dependencies = [ "serde", ] -[[package]] -name = "serde_path_to_error" -version = "0.1.20" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "10a9ff822e371bb5403e391ecd83e182e0e77ba7f6fe0160b795797109d1b457" -dependencies = [ - "itoa", - "serde", - "serde_core", -] - [[package]] name = "serde_repr" version = "0.1.20" @@ -2282,7 +2271,6 @@ dependencies = [ "log", "pretty_assertions", "serde", - "serde_path_to_error", "serde_tombi_macros", "tempfile", "textwrap", diff --git a/Cargo.toml b/Cargo.toml index 984527870..e8762b809 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -67,7 +67,6 @@ schemars = { version = "1.2.1", features = ["preserve_order", "url2"] } semver = "1.0.17" serde = { version = "1.0.219", features = ["derive"] } serde_json = { version = "1.0.140", features = ["preserve_order"] } -serde_path_to_error = "0.1.20" serde_tombi = { path = "rust/serde_tombi", default-features = false } serde_tombi_macros = { path = "rust/serde_tombi_macros" } similar = { version = "2.7.0" } diff --git a/rust/serde_tombi/Cargo.toml b/rust/serde_tombi/Cargo.toml index b10f9130c..ccadf7068 100644 --- a/rust/serde_tombi/Cargo.toml +++ b/rust/serde_tombi/Cargo.toml @@ -16,7 +16,6 @@ dirs.workspace = true itertools.workspace = true log.workspace = true serde.workspace = true -serde_path_to_error.workspace = true serde_tombi_macros.workspace = true textwrap.workspace = true thiserror.workspace = true diff --git a/rust/serde_tombi/src/de.rs b/rust/serde_tombi/src/de.rs index 3cf4d0fb2..a0168480c 100644 --- a/rust/serde_tombi/src/de.rs +++ b/rust/serde_tombi/src/de.rs @@ -109,7 +109,7 @@ impl Deserializer<'_> { where T: DeserializeOwned, { - Ok(serde_path_to_error::deserialize(&document)?) + Ok(T::deserialize(&document)?) } async fn get_toml_version( diff --git a/rust/serde_tombi/src/de/error.rs b/rust/serde_tombi/src/de/error.rs index 6a71fb4ea..20e2cb570 100644 --- a/rust/serde_tombi/src/de/error.rs +++ b/rust/serde_tombi/src/de/error.rs @@ -16,7 +16,7 @@ enum InnerError { SchemaStore(#[from] tombi_schema_store::Error), #[error(transparent)] - DocumentDeserializeWithPath(#[from] serde_path_to_error::Error), + DocumentDeserialize(#[from] tombi_document::de::Error), #[error("{}", .0.iter().map(|e| e.to_string()).collect_vec().join(", "))] Parser(Vec), @@ -61,8 +61,8 @@ impl From for Error { } } -impl From> for Error { - fn from(error: serde_path_to_error::Error) -> Self { - Self(Box::new(InnerError::DocumentDeserializeWithPath(error))) +impl From for Error { + fn from(error: tombi_document::de::Error) -> Self { + Self(Box::new(InnerError::DocumentDeserialize(error))) } } From fa51daf2f21792187eaec3da541db2b35cfd109d Mon Sep 17 00:00:00 2001 From: ya7010 Date: Thu, 6 Aug 2026 14:35:02 +0900 Subject: [PATCH 4/5] refactor: simplify logging in config file parse error handling --- rust/serde_tombi/src/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/serde_tombi/src/config.rs b/rust/serde_tombi/src/config.rs index 8c17be47a..d40607fc3 100644 --- a/rust/serde_tombi/src/config.rs +++ b/rust/serde_tombi/src/config.rs @@ -63,7 +63,7 @@ fn config_file_parse_error( config_path: &std::path::Path, error: crate::de::Error, ) -> tombi_config::Error { - log::warn!("{}", format!("Failed to parse config file {config_path:?}")); + log::warn!("Failed to parse config file {config_path:?}"); tombi_config::Error::ConfigFileParseFailed { config_path: config_path.to_owned(), From ee90ed9feb89caee7979757c05eeb261dc6be44c Mon Sep 17 00:00:00 2001 From: ya7010 Date: Thu, 6 Aug 2026 16:21:19 +0900 Subject: [PATCH 5/5] refactor: enhance error handling for config file operations by simplifying error messages and logging --- crates/tombi-config/src/error.rs | 14 ++++-------- rust/serde_tombi/src/config.rs | 39 +++++++++++++++++--------------- 2 files changed, 25 insertions(+), 28 deletions(-) diff --git a/crates/tombi-config/src/error.rs b/crates/tombi-config/src/error.rs index f47728e85..782cd6f76 100644 --- a/crates/tombi-config/src/error.rs +++ b/crates/tombi-config/src/error.rs @@ -9,17 +9,11 @@ pub enum Error { #[error("config file not found: {config_path:?}")] ConfigFileNotFound { config_path: std::path::PathBuf }, - #[error("failed to read {config_path:?}: {reason}")] - ConfigFileReadFailed { - config_path: std::path::PathBuf, - reason: String, - }, + #[error("failed to read config file {config_path:?}")] + ConfigFileReadFailed { config_path: std::path::PathBuf }, - #[error("failed to parse {config_path:?}: {reason}")] - ConfigFileParseFailed { - config_path: std::path::PathBuf, - reason: String, - }, + #[error("failed to parse config file {config_path:?}")] + ConfigFileParseFailed { config_path: std::path::PathBuf }, #[error("unsupported config file: {config_path:?}")] ConfigFileUnsupported { config_path: std::path::PathBuf }, diff --git a/rust/serde_tombi/src/config.rs b/rust/serde_tombi/src/config.rs index d40607fc3..f73dd0c69 100644 --- a/rust/serde_tombi/src/config.rs +++ b/rust/serde_tombi/src/config.rs @@ -59,16 +59,25 @@ struct Tool { } #[inline] -fn config_file_parse_error( - config_path: &std::path::Path, - error: crate::de::Error, -) -> tombi_config::Error { - log::warn!("Failed to parse config file {config_path:?}"); +fn config_file_parse_error(config_path: &std::path::Path) -> tombi_config::Error { + let error = tombi_config::Error::ConfigFileParseFailed { + config_path: config_path.to_owned(), + }; + + log::warn!("{}", error); - tombi_config::Error::ConfigFileParseFailed { + error +} + +#[inline] +fn config_file_read_error(config_path: &std::path::Path) -> tombi_config::Error { + let error = tombi_config::Error::ConfigFileReadFailed { config_path: config_path.to_owned(), - reason: error.to_string(), - } + }; + + log::warn!("{}", error); + + error } pub fn try_from_path>( @@ -82,24 +91,18 @@ pub fn try_from_path>( }); } - let config_text = std::fs::read_to_string(config_path).map_err(|error| { - log::warn!("Failed to read config file {config_path:?}"); - - tombi_config::Error::ConfigFileReadFailed { - config_path: config_path.to_owned(), - reason: error.to_string(), - } - })?; + let config_text = + std::fs::read_to_string(config_path).map_err(|_| config_file_read_error(config_path))?; match config_path.file_name().and_then(|name| name.to_str()) { Some(DOT_TOMBI_TOML_FILENAME | TOMBI_TOML_FILENAME | CONFIG_TOML_FILENAME) => { crate::config::from_str(&config_text, config_path) .map(Some) - .map_err(|error| config_file_parse_error(config_path, error)) + .map_err(|_| config_file_parse_error(config_path)) } Some(PYPROJECT_TOML_FILENAME) => { let pyproject_toml = PyProjectToml::from_str(&config_text, config_path) - .map_err(|error| config_file_parse_error(config_path, error))?; + .map_err(|_| config_file_parse_error(config_path))?; if let Some(Tool { tombi: Some(tombi_config), }) = pyproject_toml.tool