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

Some notes:

1. I would accept a table, not a file path. This will make testing much easier.
2. There is no ultra easy way to get around explicitly handling errors as far as I know. I had a similar problem in rsk and I used a macro to help a bit. See my solution there (I am pretty new to rust, so my solution might not be the best)
3. You need to get the value out using match. You can see my code for some examples.
4. To your first question, I think it is better for you to have it as a PathBuf and to do whatever you can to make that happen. PathBuf's are properly represented system paths. Your variable is a path. PathBuf is the way to go :)
5. Your second question is due to an annoying feature with unwrap I believe. The scope of the variable does not outlive default_config_file_path().to_str() or something like that. I think they are trying to fix this.