From ed89c8abe7e73a6a4200376b056a95fe2a845be4 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sun, 8 Dec 2024 22:31:09 +0900 Subject: [PATCH] config: add ConfigLoadError, replace uses of config::ConfigError --- cli/src/command_error.rs | 22 ++++++++++----- cli/src/config.rs | 8 +++--- cli/tests/test_fix_command.rs | 6 +++-- cli/tests/test_global_opts.rs | 6 +++-- lib/src/config.rs | 51 ++++++++++++++++++++++------------- 5 files changed, 59 insertions(+), 34 deletions(-) diff --git a/cli/src/command_error.rs b/cli/src/command_error.rs index d98d9acf9b..82949760aa 100644 --- a/cli/src/command_error.rs +++ b/cli/src/command_error.rs @@ -22,8 +22,8 @@ use std::sync::Arc; use itertools::Itertools as _; use jj_lib::backend::BackendError; -use jj_lib::config::ConfigError; use jj_lib::config::ConfigGetError; +use jj_lib::config::ConfigLoadError; use jj_lib::dsl_util::Diagnostics; use jj_lib::fileset::FilePatternParseError; use jj_lib::fileset::FilesetParseError; @@ -240,12 +240,6 @@ impl From for CommandError { } } -impl From for CommandError { - fn from(err: ConfigError) -> Self { - config_error(err) - } -} - impl From for CommandError { fn from(err: ConfigEnvError) -> Self { config_error(err) @@ -266,6 +260,20 @@ impl From for CommandError { } } +impl From for CommandError { + fn from(err: ConfigLoadError) -> Self { + let hint = match &err { + ConfigLoadError::Read(_) => None, + ConfigLoadError::Parse { source_path, .. } => source_path + .as_ref() + .map(|path| format!("Check the config file: {}", path.display())), + }; + let mut cmd_err = config_error(err); + cmd_err.extend_hints(hint); + cmd_err + } +} + impl From for CommandError { fn from(err: RewriteRootCommit) -> Self { internal_error_with_message("Attempted to rewrite the root commit", err) diff --git a/cli/src/config.rs b/cli/src/config.rs index 34ca699b48..fd25038c50 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -22,8 +22,8 @@ use std::path::PathBuf; use std::process::Command; use itertools::Itertools; -use jj_lib::config::ConfigError; use jj_lib::config::ConfigLayer; +use jj_lib::config::ConfigLoadError; use jj_lib::config::ConfigNamePathBuf; use jj_lib::config::ConfigSource; use jj_lib::config::ConfigValue; @@ -263,7 +263,7 @@ impl ConfigEnv { /// Loads user-specific config files into the given `config`. The old /// user-config layers will be replaced if any. #[instrument] - pub fn reload_user_config(&self, config: &mut StackedConfig) -> Result<(), ConfigError> { + pub fn reload_user_config(&self, config: &mut StackedConfig) -> Result<(), ConfigLoadError> { config.remove_layers(ConfigSource::User); if let Some(path) = self.existing_user_config_path() { if path.is_dir() { @@ -301,7 +301,7 @@ impl ConfigEnv { /// Loads repo-specific config file into the given `config`. The old /// repo-config layer will be replaced if any. #[instrument] - pub fn reload_repo_config(&self, config: &mut StackedConfig) -> Result<(), ConfigError> { + pub fn reload_repo_config(&self, config: &mut StackedConfig) -> Result<(), ConfigLoadError> { config.remove_layers(ConfigSource::Repo); if let Some(path) = self.existing_repo_config_path() { config.load_file(ConfigSource::Repo, path)?; @@ -403,7 +403,7 @@ fn env_overrides_layer() -> ConfigLayer { } /// Parses `--config-toml` arguments. -pub fn parse_config_args(toml_strs: &[String]) -> Result, ConfigError> { +pub fn parse_config_args(toml_strs: &[String]) -> Result, ConfigLoadError> { // It might look silly that a layer is constructed per argument, but // --config-toml argument can contain a full TOML document, and it makes // sense to preserve line numbers within the doc. If we add diff --git a/cli/tests/test_fix_command.rs b/cli/tests/test_fix_command.rs index c822efccfe..3d00724bd9 100644 --- a/cli/tests/test_fix_command.rs +++ b/cli/tests/test_fix_command.rs @@ -171,13 +171,15 @@ fn test_config_multiple_tools_with_same_name() { let stderr = test_env.jj_cmd_failure(&repo_path, &["fix"]); insta::assert_snapshot!(stderr, @r" - Config error: TOML parse error at line 6, column 9 + Config error: Configuration cannot be parsed as TOML document + Caused by: TOML parse error at line 6, column 9 | 6 | [fix.tools.my-tool] | ^ invalid table header duplicate key `my-tool` in table `fix.tools` - in $TEST_ENV/config/config0002.toml + + Hint: Check the config file: $TEST_ENV/config/config0002.toml For help, see https://martinvonz.github.io/jj/latest/config/. "); diff --git a/cli/tests/test_global_opts.rs b/cli/tests/test_global_opts.rs index 5456963ed6..b3573211df 100644 --- a/cli/tests/test_global_opts.rs +++ b/cli/tests/test_global_opts.rs @@ -587,13 +587,15 @@ fn test_invalid_config() { test_env.add_config("[section]key = value-missing-quotes"); let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["init", "repo"]); insta::assert_snapshot!(stderr, @r" - Config error: TOML parse error at line 1, column 10 + Config error: Configuration cannot be parsed as TOML document + Caused by: TOML parse error at line 1, column 10 | 1 | [section]key = value-missing-quotes | ^ invalid table header expected newline, `#` - in $TEST_ENV/config/config0002.toml + + Hint: Check the config file: $TEST_ENV/config/config0002.toml For help, see https://martinvonz.github.io/jj/latest/config/. "); } diff --git a/lib/src/config.rs b/lib/src/config.rs index 0b37a2ee41..65a30320a8 100644 --- a/lib/src/config.rs +++ b/lib/src/config.rs @@ -32,6 +32,7 @@ use toml_edit::DocumentMut; use toml_edit::ImDocument; use crate::file_util::IoResultExt as _; +use crate::file_util::PathError; /// Config value or table node. pub type ConfigItem = toml_edit::Item; @@ -40,9 +41,22 @@ pub type ConfigTable = toml_edit::Table; /// Generic config value. pub type ConfigValue = toml_edit::Value; -/// Error that can occur when accessing configuration. -// TODO: will be replaced with our custom error type -pub type ConfigError = config::ConfigError; +/// Error that can occur when parsing or loading config variables. +#[derive(Debug, Error)] +pub enum ConfigLoadError { + /// Config file or directory cannot be read. + #[error("Failed to read configuration file")] + Read(#[source] PathError), + /// TOML file or text cannot be parsed. + #[error("Configuration cannot be parsed as TOML document")] + Parse { + /// Source error. + #[source] + error: toml_edit::TomlError, + /// Source file path. + source_path: Option, + }, +} /// Error that can occur when looking up config variable. #[derive(Debug, Error)] @@ -270,22 +284,21 @@ impl ConfigLayer { } /// Parses TOML document `text` into new layer. - pub fn parse(source: ConfigSource, text: &str) -> Result { - let data = ImDocument::parse(text).map_err(|err| ConfigError::FileParse { - uri: None, - cause: err.into(), + pub fn parse(source: ConfigSource, text: &str) -> Result { + let data = ImDocument::parse(text).map_err(|error| ConfigLoadError::Parse { + error, + source_path: None, })?; Ok(Self::with_data(source, data.into_mut())) } - fn load_from_file(source: ConfigSource, path: PathBuf) -> Result { - let text = fs::read_to_string(&path).map_err(|err| ConfigError::FileParse { - uri: Some(path.to_string_lossy().into_owned()), - cause: err.into(), - })?; - let data = ImDocument::parse(text).map_err(|err| ConfigError::FileParse { - uri: Some(path.to_string_lossy().into_owned()), - cause: err.into(), + fn load_from_file(source: ConfigSource, path: PathBuf) -> Result { + let text = fs::read_to_string(&path) + .context(&path) + .map_err(ConfigLoadError::Read)?; + let data = ImDocument::parse(text).map_err(|error| ConfigLoadError::Parse { + error, + source_path: Some(path.clone()), })?; Ok(ConfigLayer { source, @@ -294,7 +307,7 @@ impl ConfigLayer { }) } - fn load_from_dir(source: ConfigSource, path: &Path) -> Result, ConfigError> { + fn load_from_dir(source: ConfigSource, path: &Path) -> Result, ConfigLoadError> { // TODO: Walk the directory recursively? let mut file_paths: Vec<_> = path .read_dir() @@ -306,7 +319,7 @@ impl ConfigLayer { .try_collect() }) .context(path) - .map_err(|err| ConfigError::Foreign(err.into()))?; + .map_err(ConfigLoadError::Read)?; file_paths.sort_unstable(); file_paths .into_iter() @@ -431,7 +444,7 @@ impl StackedConfig { &mut self, source: ConfigSource, path: impl Into, - ) -> Result<(), ConfigError> { + ) -> Result<(), ConfigLoadError> { let layer = ConfigLayer::load_from_file(source, path.into())?; self.add_layer(layer); Ok(()) @@ -443,7 +456,7 @@ impl StackedConfig { &mut self, source: ConfigSource, path: impl AsRef, - ) -> Result<(), ConfigError> { + ) -> Result<(), ConfigLoadError> { let layers = ConfigLayer::load_from_dir(source, path.as_ref())?; self.extend_layers(layers); Ok(())