Improve error from bad escapes in a toml config.
Whereas previously we would say: ``` target/debug/arti: error: invalid escape character in string: `Z` at line 9 column 14 in ../../.config/arti/arti.toml ``` we now say: ``` target/debug/arti: error: invalid escape character in string: `Z` at line 9 column 14 in ../../.config/arti/arti.toml (If you wanted to include a literal \ character, you need to escape it by writing two in a row: \\) ``` The implementation is a bit of a hack, I'm afraid, but I don't think it's all that bad. Closes #549.
This commit is contained in:
parent
cbb30b5e75
commit
88fa24d029
|
@ -8,3 +8,5 @@ BREAKING: load::resolve_ignore_unrecognized renamed resolve_ignore_warnings
|
|||
BREAKING: load::resolve_return_unrecognized replaced with resolve_return_results
|
||||
BREAKING: load::UnrecognizedKey renamed to DisfavouredKey
|
||||
ADDED: Support for tracking deprecated config keys
|
||||
BREAKING: Return different error type on load().
|
||||
|
||||
|
|
|
@ -1,5 +1,7 @@
|
|||
//! Declare error types.
|
||||
|
||||
use std::sync::Arc;
|
||||
|
||||
use tor_error::{ErrorKind, HasKind};
|
||||
|
||||
/// An error related to an option passed to Arti via a configuration
|
||||
|
@ -102,6 +104,40 @@ impl HasKind for ReconfigureError {
|
|||
}
|
||||
}
|
||||
|
||||
/// Wrapper for [`config::ConfigError`] with a more helpful error message.
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct ConfigError(Arc<config::ConfigError>);
|
||||
|
||||
impl From<config::ConfigError> for ConfigError {
|
||||
fn from(err: config::ConfigError) -> Self {
|
||||
ConfigError(Arc::new(err))
|
||||
}
|
||||
}
|
||||
|
||||
impl std::fmt::Display for ConfigError {
|
||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||
let s = self.0.to_string();
|
||||
write!(f, "{}", s)?;
|
||||
if s.contains("invalid escape") || s.contains("invalid hex escape") {
|
||||
write!(f, " (If you wanted to include a literal \\ character, you need to escape it by writing two in a row: \\\\)")?;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
impl std::error::Error for ConfigError {
|
||||
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
|
||||
Some(&self.0)
|
||||
}
|
||||
}
|
||||
|
||||
impl ConfigError {
|
||||
/// Return the inner [`config::ConfigError`] that this is wrapping.
|
||||
pub fn inner(&self) -> &config::ConfigError {
|
||||
self.0.as_ref()
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod test {
|
||||
#![allow(clippy::unwrap_used)]
|
||||
|
|
|
@ -105,7 +105,7 @@ pub mod sources;
|
|||
pub use cmdline::CmdLine;
|
||||
pub use config as config_crate;
|
||||
pub use educe;
|
||||
pub use err::{ConfigBuildError, ReconfigureError};
|
||||
pub use err::{ConfigBuildError, ConfigError, ReconfigureError};
|
||||
pub use itertools::Itertools;
|
||||
pub use load::{resolve, resolve_ignore_warnings, resolve_return_results};
|
||||
pub use misc::*;
|
||||
|
|
|
@ -104,13 +104,19 @@ use crate::ConfigBuildError;
|
|||
pub enum ConfigResolveError {
|
||||
/// Deserialize failed
|
||||
#[error("Config contents not as expected")]
|
||||
Deserialize(#[from] config::ConfigError),
|
||||
Deserialize(#[from] crate::ConfigError),
|
||||
|
||||
/// Build failed
|
||||
#[error("Config semantically incorrect")]
|
||||
Build(#[from] ConfigBuildError),
|
||||
}
|
||||
|
||||
impl From<config::ConfigError> for ConfigResolveError {
|
||||
fn from(err: config::ConfigError) -> Self {
|
||||
crate::ConfigError::from(err).into()
|
||||
}
|
||||
}
|
||||
|
||||
/// A type that can be built from a builder via a build method
|
||||
pub trait Builder {
|
||||
/// The type that this builder constructs
|
||||
|
|
|
@ -25,10 +25,9 @@
|
|||
use std::ffi::OsString;
|
||||
use std::{fs, io};
|
||||
|
||||
use crate::err::ConfigError;
|
||||
use crate::CmdLine;
|
||||
|
||||
use config::ConfigError;
|
||||
|
||||
/// The synchronous configuration builder type we use.
|
||||
///
|
||||
/// (This is a type alias that config should really provide.)
|
||||
|
@ -242,16 +241,13 @@ impl ConfigurationSources {
|
|||
// or Ok(()) if we should ignore the error and skip the file.
|
||||
let handle_io_error = |e: io::Error| {
|
||||
if e.kind() == io::ErrorKind::NotFound && !required {
|
||||
Ok(())
|
||||
Result::<_, crate::ConfigError>::Ok(())
|
||||
} else {
|
||||
Err(ConfigError::Foreign(
|
||||
anyhow::anyhow!(format!(
|
||||
"unable to access config path: {:?}: {}",
|
||||
&source.as_path(),
|
||||
e
|
||||
))
|
||||
.into(),
|
||||
))
|
||||
Err(foreign_err(anyhow::anyhow!(format!(
|
||||
"unable to access config path: {:?}: {}",
|
||||
&source.as_path(),
|
||||
e
|
||||
))))
|
||||
}
|
||||
};
|
||||
|
||||
|
@ -341,7 +337,7 @@ impl FoundConfigFiles<'_> {
|
|||
{
|
||||
Ok(()) => {}
|
||||
Err(fs_mistrust::Error::NotFound(_)) if !required => {}
|
||||
Err(e) => return Err(ConfigError::Foreign(e.into())),
|
||||
Err(e) => return Err(foreign_err(e)),
|
||||
}
|
||||
|
||||
// Not going to use File::with_name here, since it doesn't
|
||||
|
@ -363,7 +359,7 @@ impl FoundConfigFiles<'_> {
|
|||
pub fn load(self) -> Result<config::Config, ConfigError> {
|
||||
let mut builder = config::Config::builder();
|
||||
builder = self.add_sources(builder)?;
|
||||
builder.build()
|
||||
Ok(builder.build()?)
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -393,6 +389,14 @@ fn is_syntactically_directory(p: &Path) -> bool {
|
|||
}
|
||||
}
|
||||
|
||||
/// Convert an error `E` into a [`ConfigError`](crate::ConfigErr).
|
||||
fn foreign_err<E>(err: E) -> crate::ConfigError
|
||||
where
|
||||
E: Into<Box<dyn std::error::Error + Send + Sync>>,
|
||||
{
|
||||
crate::ConfigError::from(config::ConfigError::Foreign(err.into()))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod test {
|
||||
#![allow(clippy::unwrap_used)]
|
||||
|
@ -429,7 +433,7 @@ friends = 4242
|
|||
fn load_nodefaults<P: AsRef<Path>>(
|
||||
files: &[(P, MustRead)],
|
||||
opts: &[String],
|
||||
) -> Result<config::Config, config::ConfigError> {
|
||||
) -> Result<config::Config, crate::ConfigError> {
|
||||
sources_nodefaults(files, opts).load()
|
||||
}
|
||||
|
||||
|
|
Loading…
Reference in New Issue