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.

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

Typed Config Access #5552

Merged
merged 7 commits into from
May 30, 2018
Merged
Changes from 1 commit
Commits
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
41 changes: 22 additions & 19 deletions src/cargo/util/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -847,21 +847,21 @@ impl fmt::Display for ConfigKey {
/// Internal error for serde errors.
#[derive(Debug)]
pub struct ConfigError {
message: String,
error: failure::Error,
Copy link
Member

Choose a reason for hiding this comment

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

We have pub use failure::Error as CargoError somewhere. I don't know if we want to use failure::Error or CargoError, but currently we use CargoError in the majority of the code base.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kinda assumed that was there just for being compatible with the old CargoError which used to be its own type. @alexcrichton was it intended to continue to maintain that abstraction? The use of failure::Error is being used in about 4 other places, so it is leaking a little.

Copy link
Member

Choose a reason for hiding this comment

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

Oh that was actually a holdover to make the "migrate to failure" patch smaller than it otherwise would be. Nowadays we should just switch to failure::Error everywhere I think, the transition to failure just isn't fully done yet

definition: Option<Definition>,
}

impl ConfigError {
fn new(message: String, definition: Definition) -> ConfigError {
ConfigError {
message,
error: failure::err_msg(message),
definition: Some(definition),
}
}

fn expected(key: &str, expected: &str, found: &ConfigValue) -> ConfigError {
ConfigError {
message: format!(
error: format_err!(
"`{}` expected {}, but found a {}",
key,
expected,
Expand All @@ -873,56 +873,59 @@ impl ConfigError {

fn missing(key: String) -> ConfigError {
ConfigError {
message: format!("missing config key `{}`", key),
error: format_err!("missing config key `{}`", key),
definition: None,
}
}

fn with_key_context(self, key: String, definition: Definition) -> ConfigError {
ConfigError {
message: format!("could not load config key `{}`: {}", key, self),
error: format_err!("could not load config key `{}`: {}", key, self),
definition: Some(definition),
}
}
}

impl std::error::Error for ConfigError {
// This can be removed once 1.27 is stable.
fn description(&self) -> &str {
self.message.as_str()
"An error has occurred."
}
}

// Future Note: Currently we cannot override Fail::cause (due to
// specialization) so we have no way to return the underlying causes. In the
// future, once this limitation is lifted, this should instead implement
// `cause` and avoid doing the cause formatting here.
impl fmt::Display for ConfigError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
let message = self
.error
.causes()
.map(|e| e.to_string())
.collect::<Vec<_>>()
.join("\nCaused by:\n ");
if let Some(ref definition) = self.definition {
write!(f, "error in {}: {}", definition, self.message)
write!(f, "error in {}: {}", definition, message)
} else {
self.message.fmt(f)
message.fmt(f)
}
}
}

impl de::Error for ConfigError {
fn custom<T: fmt::Display>(msg: T) -> Self {
ConfigError {
message: msg.to_string(),
error: failure::err_msg(msg.to_string()),
definition: None,
}
}
}

// TODO: AFAIK, you cannot override Fail::cause (due to specialization), so we
// have no way to bubble up the underlying error. For now, it just formats
// the underlying cause as a string.
impl From<failure::Error> for ConfigError {
fn from(e: failure::Error) -> Self {
let message = e
.causes()
.map(|e| e.to_string())
.collect::<Vec<_>>()
.join("\nCaused by:\n ");
fn from(error: failure::Error) -> Self {
ConfigError {
message,
error,
definition: None,
}
}
Expand Down