[3/3] Rewrite settings parsing to avoid panic

Change parse() to return a Result, using the question mark operator to
catch errors and return early.

Use map_err() to turn the low-level errors into something human
readable. We could define our own error type but that seems like
overkill for this.

Rework the tests to cope. For the two failing cases that's a bit ugly
because we have to map the Ok() to Err() and vice versa.

There are now no panic()s or unwrap()s in settings.rs!

Error messages seen by the user look like, eg:
  Error: Unable to parse config file: missing field `git`
  Error: Unable to open config file: Permission denied (os error 13)
  Error: Unable to open config file: No such file or directory (os error 2)
---
 src/main.rs     |  2 +-
 src/settings.rs | 42 +++++++++++++++++++++---------------------
 2 files changed, 22 insertions(+), 22 deletions(-)

On 13/12/18 1:43 pm, Michael Ellerman wrote:
> Change parse() to return a Result, using the question mark operator to
> catch errors and return early.
> 
> Use map_err() to turn the low-level errors into something human
> readable. We could define our own error type but that seems like
> overkill for this.
> 
> Rework the tests to cope. For the two failing cases that's a bit ugly
> because we have to map the Ok() to Err() and vice versa.
> 
> There are now no panic()s or unwrap()s in settings.rs!
> 
> Error messages seen by the user look like, eg:
>    Error: Unable to parse config file: missing field `git`
>    Error: Unable to open config file: Permission denied (os error 13)
>    Error: Unable to open config file: No such file or directory (os error 2)

woo!

Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

> ---
>   src/main.rs     |  2 +-
>   src/settings.rs | 42 +++++++++++++++++++++---------------------
>   2 files changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/src/main.rs b/src/main.rs
> index 2e022a3939e9..d2c24be3a59f 100644
> --- a/src/main.rs
> +++ b/src/main.rs
> @@ -303,7 +303,7 @@ fn run() -> Result<(), Box<Error>> {
>           .and_then(|d| d.version(Some(version)).deserialize())
>           .unwrap_or_else(|e| e.exit());
>   
> -    let settings = settings::parse(&args.arg_config_file);
> +    let settings = settings::parse(&args.arg_config_file)?;
>   
>       // The HTTP client we'll use to access the APIs
>       // TODO: Handle https_proxy
> diff --git a/src/settings.rs b/src/settings.rs
> index a5513728fbaa..8e86b213a6bc 100644
> --- a/src/settings.rs
> +++ b/src/settings.rs
> @@ -22,6 +22,7 @@ use git2;
>   use git2::Repository;
>   
>   use std::collections::BTreeMap;
> +use std::error::Error;
>   use std::fmt;
>   use std::fs::File;
>   use std::io::Read;
> @@ -186,23 +187,18 @@ pub struct Config {
>       pub projects: BTreeMap<String, Project>,
>   }
>   
> -pub fn parse(path: &str) -> Config {
> +pub fn parse(path: &str) -> Result<Config, Box<Error>> {
>       let mut toml_config = String::new();
>   
> -    let mut file = File::open(&path).expect("Couldn't open config file, exiting.");
> +    File::open(&path)
> +        .map_err(|e| format!("Unable to open config file: {}", e))?
> +        .read_to_string(&mut toml_config)
> +        .map_err(|e| format!("Unable to read config file: {}", e))?;
>   
> -    file.read_to_string(&mut toml_config)
> -        .unwrap_or_else(|err| panic!("Couldn't read config: {}", err));
> +    let config = toml::de::from_str::<Config>(&toml_config)
> +        .map_err(|e| format!("Unable to parse config file: {}", e))?;
>   
> -    let toml_config = toml::de::from_str::<Config>(&toml_config);
> -
> -    match toml_config {
> -        Ok(config_inside) => config_inside,
> -        Err(err) => {
> -            error!("TOML error: {}", err);
> -            panic!("Could not parse configuration file, exiting");
> -        }
> -    }
> +    Ok(config)
>   }
>   
>   #[cfg(test)]
> @@ -210,19 +206,23 @@ mod test {
>       use settings::*;
>   
>       #[test]
> -    #[should_panic(expected = "Couldn't open config file")]
> -    fn bad_path() {
> -        parse("/nonexistent/config.file");
> +    fn bad_path() -> Result<(), &'static str> {
> +        match parse("/nonexistent/config.file") {
> +            Ok(_) => Err("Didn't fail opening non-existent file?"),
> +            Err(_) => Ok(()),
> +        }
>       }
>   
>       #[test]
> -    fn parse_example_openpower() {
> -        parse("examples/openpower.toml");
> +    fn parse_example_openpower() -> Result<(), Box<Error>> {
> +        parse("examples/openpower.toml").map(|_| ())
>       }
>   
>       #[test]
> -    #[should_panic(expected = "Could not parse configuration file, exiting")]
> -    fn parse_example_invalid() {
> -        parse("examples/tests/invalid.toml");
> +    fn parse_example_invalid() -> Result<(), &'static str> {
> +        match parse("examples/tests/invalid.toml") {
> +            Ok(_) => Err("Didn't fail parsing invalid TOML?"),
> +            Err(_) => Ok(())
> +        }
>       }
>   }
>