eightbitraptor/rust_review.md

## rust_review.md

      
    Raw
  

              rust_review.md
            
          
    Code Review Request!

I am building a command line application. Part of the application relies on a configuration file. The intended behaviour is to have this configuration file as a dotfile in the users home directory. The configuration files are written using TOML.
The cli ui has a -c/--config option that the user can use to provide the path to an alternate configuration file.
If the -c parameter is used the application must read the provided config file and parse it otherwise it will fall back to ~/.application.toml
The configuration file is parsed and a Config struct is built that maps the values in the config to fields on the struct. At the moment, there is only one param in the config file: data_path. so the Struct is very simple.
#[derive(Debug)]
pub struct Config {
    pub data_path: String,
}

It has an impl that contains a single method from_file that handles the parsing and building of a Config object. That method looks like this
pub fn from_file(config_file_path: &str) -> Result<Config, Error> {
    let mut raw_config = String::new();

    try!(File::open(config_file_path)
        .and_then(|mut f| f.read_to_string(&mut raw_config)));

    let mut parser = toml::Parser::new(&raw_config);
    let parsed_toml = match parser.parse() {
        Some(toml) => toml,
        None => panic!("Invalid TOML in config file:{}\n{:?}", &parser.errors),
    };
    let myapp_options = parsed_toml.get("myapp").unwrap();
    let data_path = myapp_options.lookup("data_path").unwrap().as_str();
    let config = Config { data_path: String::from(data_path.unwrap()) };

    Ok(config)
}

Questions:


I feel like I'm using unwrap() a lot. Especially when pulling data out of the config file. This is probably going to fail when a config file is found, and readable, but doesn't contain the correct data. Equally replacing unwrap calls with unwrap_or_else could make this method unneccessarily verbose - What is the best Rusty way to do error handling in this case?


looking up the data_path from TOML returns a toml::Value enum type (http://alexcrichton.com/toml-rs/toml/enum.Value.html) This has a bunch of variants one of which is String. I feel like I can get the String out of this in a more efficient way than using as_str followed by String::from but I'm not sure what it is.


I fixed this by reading the documentation for toml::Value a little better :S - Replaced with the following, which I like a lot better
let data_path: String = myapp_options.lookup("data_path")
        .and_then(|path_value| path_value.as_str())
        .expect("config file must define data_path as a string")
        .to_owned();

    let config = Config { data_path: data_path };

This code is being used from my main application cli layer as follows
fn main() {
    // ...snip
    let default_config = default_config_file_path();
    let config_file_name = arguments.value_of("config")
        .unwrap_or(default_config.to_str().unwrap());

    let configuration = Config::from_file(config_file_name)
        .unwrap_or_else(|err| {
            panic!("Something went wrong reading config file {}\n{}",
                config_file_name,
                err)
            });

    // ...snip
}

The default_config_file_path function uses std::env::home_dir and some joining methods to return a PathBuf that contains /home/me/.application.toml which we use as the default config_file_name if one was not provided. As an aside, I really like this use of unwrap_or it makes the code very readable and clear.
Questions


Again with the unwrap calls. What I really wanted was for the default_config_file_path function to return a str rather than a PathBuf so that I didn't have to use the as_str().unwrap() chain, which makes the config_file_name binding much cleaner, ie arguments.value_of("config").unwrap_or(default_config);. I could not work out how to make this happen. I kept getting trait bound std::marker::Sized is not satisfied - I don't know what the correct idiomatic way to do this sort of thing is.


Why do I have to call default_config_file_path and bind the result to the default_config variable? When I try and use that method in place I get a compile error because:
  Compiling myapp v0.1.0 (file:///Users/matth/code/projects/myapp)
  src/bin/myapp.rs:35:20: 35:46 error: borrowed value does not live long enough
  src/bin/myapp.rs:35         .unwrap_or(default_config_file_path().to_str().unwrap());
  	                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~
  src/bin/myapp.rs:35:66: 54:2 note: reference must be valid for the block suffix following statement 4 at 35:65...


Update

I decided to publish the source code, even though it's merely a learning exercise at this point:
https://github.com/eightbitraptor/otaku