Provide config.set().item(value) API.

This API isn't fantastic, but it's the best I can come up with without
something like `concat_idents!()`. There are relatively few places where
config is set, to hopefully the ugliness isn't disastrous.

Change previous occurences of `config.item = value` to this new API,
rather than using `config.override_value()`. Undo the changes to
`override_value()`, as it's no longer important to propogate the error
to the caller. Add a test for the new interface.

src/bin/rustfmt.rs

@@ -18,12 +18,14 @@ extern crate env_logger;
 extern crate getopts;
 
 use rustfmt::{run, Input, Summary};
-use rustfmt::config::Config;
+use rustfmt::file_lines::FileLines;
+use rustfmt::config::{Config, WriteMode};
 
 use std::{env, error};
 use std::fs::{self, File};
 use std::io::{self, ErrorKind, Read, Write};
 use std::path::{Path, PathBuf};
+use std::str::FromStr;
 
 use getopts::{Matches, Options};
 
@@ -61,8 +63,8 @@ enum Operation {
 struct CliOptions {
     skip_children: bool,
     verbose: bool,
-    write_mode: Option<String>,
+    write_mode: Option<WriteMode>,
-    file_lines: Option<String>,
+    file_lines: FileLines, // Default is all lines in all files.
 }
 
 impl CliOptions {
@@ -71,29 +73,28 @@ impl CliOptions {
         options.skip_children = matches.opt_present("skip-children");
         options.verbose = matches.opt_present("verbose");
 
-        if let Some(write_mode) = matches.opt_str("write-mode") {
+        if let Some(ref write_mode) = matches.opt_str("write-mode") {
-            options.write_mode = Some(write_mode);
+            if let Ok(write_mode) = WriteMode::from_str(write_mode) {
+                options.write_mode = Some(write_mode);
+            } else {
+                return Err(FmtError::from(format!("Invalid write-mode: {}", write_mode)));
+            }
         }
 
-        if let Some(file_lines) = matches.opt_str("file-lines") {
+        if let Some(ref file_lines) = matches.opt_str("file-lines") {
-            options.file_lines = Some(file_lines);
+            options.file_lines = file_lines.parse()?;
         }
 
         Ok(options)
     }
 
-    fn apply_to(&self, config: &mut Config) -> FmtResult<()> {
+    fn apply_to(self, config: &mut Config) {
-        let bool_to_str = |b| if b { "true" } else { "false" };
+        config.set().skip_children(self.skip_children);
-        config
+        config.set().verbose(self.verbose);
-            .override_value("skip_children", bool_to_str(self.skip_children))?;
+        config.set().file_lines(self.file_lines);
-        config.override_value("verbose", bool_to_str(self.verbose))?;
+        if let Some(write_mode) = self.write_mode {
-        if let Some(ref write_mode) = self.write_mode {
+            config.set().write_mode(write_mode);
-            config.override_value("write_mode", &write_mode)?;
         }
-        if let Some(ref file_lines) = self.file_lines {
-            config.override_value("file_lines", &file_lines)?;
-        }
-        Ok(())
     }
 }
 
@@ -221,11 +222,11 @@ fn execute(opts: &Options) -> FmtResult<Summary> {
                                                          &env::current_dir().unwrap())?;
 
             // write_mode is always Plain for Stdin.
-            config.override_value("write_mode", "Plain")?;
+            config.set().write_mode(WriteMode::Plain);
 
             // parse file_lines
             if let Some(ref file_lines) = matches.opt_str("file-lines") {
-                config.override_value("file-lines", file_lines)?;
+                config.set().file_lines(file_lines.parse()?);
                 for f in config.file_lines().files() {
                     if f != "stdin" {
                         println!("Warning: Extra file listed in file_lines option '{}'", f);
@@ -238,6 +239,12 @@ fn execute(opts: &Options) -> FmtResult<Summary> {
         Operation::Format { files, config_path } => {
             let options = CliOptions::from_matches(&matches)?;
 
+            for f in options.file_lines.files() {
+                if !files.contains(&PathBuf::from(f)) {
+                    println!("Warning: Extra file listed in file_lines option '{}'", f);
+                }
+            }
+
             let mut config = Config::default();
             let mut path = None;
             // Load the config path file if provided
@@ -246,13 +253,6 @@ fn execute(opts: &Options) -> FmtResult<Summary> {
                 config = cfg_tmp;
                 path = path_tmp;
             };
-            options.apply_to(&mut config)?;
-
-            for f in config.file_lines().files() {
-                if !files.contains(&PathBuf::from(f)) {
-                    println!("Warning: Extra file listed in file_lines option '{}'", f);
-                }
-            }
 
             if options.verbose {
                 if let Some(path) = path.as_ref() {
@@ -282,7 +282,7 @@ fn execute(opts: &Options) -> FmtResult<Summary> {
                         config = config_tmp;
                     }
 
-                    options.apply_to(&mut config)?;
+                    options.clone().apply_to(&mut config);
                     error_summary.add(run(Input::File(file), &config));
                 }
             }

src/comment.rs

@@ -731,10 +731,8 @@ mod test {
     #[cfg_attr(rustfmt, rustfmt_skip)]
     fn format_comments() {
         let mut config: ::config::Config = Default::default();
-        config.override_value("wrap_comments", "true")
+        config.set().wrap_comments(true);
-            .expect("Could not set wrap_comments to true");
+        config.set().normalize_comments(true);
-        config.override_value("normalize_comments", "true")
-            .expect("Could not set normalize_comments to true");
 
         let comment = rewrite_comment(" //test",
                                       true,

src/config.rs

@@ -11,8 +11,6 @@
 extern crate toml;
 
 use std::cell::Cell;
-use std::error;
-use std::result;
 
 use file_lines::FileLines;
 use lists::{SeparatorTactic, ListTactic};
@@ -231,6 +229,21 @@ macro_rules! create_config {
             $(pub $i: Option<$ty>),+
         }
 
+        // Macro hygiene won't allow us to make `set_$i()` methods on Config
+        // for each item, so this struct is used to give the API to set values:
+        // `config.get().option(false)`. It's pretty ugly. Consider replacing
+        // with `config.set_option(false)` if we ever get a stable/usable
+        // `concat_idents!()`.
+        pub struct ConfigSetter<'a>(&'a mut Config);
+
+        impl<'a> ConfigSetter<'a> {
+            $(
+            pub fn $i(&mut self, value: $ty) {
+                (self.0).$i.1 = value;
+            }
+            )+
+        }
+
         impl Config {
 
             $(
@@ -240,6 +253,10 @@ macro_rules! create_config {
             }
             )+
 
+            pub fn set<'a>(&'a mut self) -> ConfigSetter<'a> {
+                ConfigSetter(self)
+            }
+
             fn fill_from_parsed_config(mut self, parsed: PartialConfig) -> Config {
             $(
                 if let Some(val) = parsed.$i {
@@ -316,15 +333,19 @@ macro_rules! create_config {
             }
 
             pub fn override_value(&mut self, key: &str, val: &str)
-                -> result::Result<(), Box<error::Error + Send + Sync>>
             {
                 match key {
                     $(
-                        stringify!($i) => self.$i.1 = val.parse::<$ty>()?,
+                        stringify!($i) => {
+                            self.$i.1 = val.parse::<$ty>()
+                                .expect(&format!("Failed to parse override for {} (\"{}\") as a {}",
+                                                 stringify!($i),
+                                                 val,
+                                                 stringify!($ty)));
+                        }
                     )+
                     _ => panic!("Unknown config key in override: {}", key)
                 }
-                Ok(())
             }
 
             pub fn print_docs() {
@@ -480,4 +501,13 @@ mod test {
         assert!(config.skip_children.0.get());
         assert!(!config.disable_all_formatting.0.get());
     }
+
+    #[test]
+    fn test_config_set() {
+        let mut config = Config::default();
+        config.set().verbose(false);
+        assert_eq!(config.verbose(), false);
+        config.set().verbose(true);
+        assert_eq!(config.verbose(), true);
+    }
 }

src/lib.rs

@@ -708,9 +708,7 @@ mod test {
     #[test]
     fn indent_to_string_hard_tabs() {
         let mut config = Config::default();
-        config
+        config.set().hard_tabs(true);
-            .override_value("hard_tabs", "true")
-            .expect("Could not set hard_tabs to true");
         let indent = Indent::new(8, 4);
 
         // 2 tabs + 4 spaces

tests/system.rs

@@ -20,7 +20,7 @@ use std::path::{Path, PathBuf};
 
 use rustfmt::*;
 use rustfmt::filemap::{write_system_newlines, FileMap};
-use rustfmt::config::Config;
+use rustfmt::config::{Config, ReportTactic};
 use rustfmt::rustfmt_diff::*;
 
 const DIFF_CONTEXT_SIZE: usize = 3;
@@ -224,16 +224,12 @@ fn read_config(filename: &str) -> Config {
 
     for (key, val) in &sig_comments {
         if key != "target" && key != "config" {
-            config
+            config.override_value(key, val);
-                .override_value(key, val)
-                .expect(&format!("Failed to override config {} (\"{}\")", key, val));
         }
     }
 
     // Don't generate warnings for to-do items.
-    config
+    config.set().report_todo(ReportTactic::Never);
-        .override_value("report_todo", "Never")
-        .expect("Could not set report-todo to Never");
 
     config
 }