Refactor CliOptions

src/bin/main.rs

@@ -11,6 +11,7 @@
 #![cfg(not(test))]
 
 extern crate env_logger;
+#[macro_use]
 extern crate failure;
 extern crate getopts;
 extern crate rustfmt_nightly as rustfmt;
@@ -19,14 +20,15 @@
 use std::fs::File;
 use std::io::{self, stdout, Read, Write};
 use std::path::{Path, PathBuf};
+use std::str::FromStr;
 
 use failure::err_msg;
 
 use getopts::{Matches, Options};
 
 use rustfmt::{
-    emit_post_matter, emit_pre_matter, format_and_emit_report, load_config, CliOptions, Config,
+    emit_post_matter, emit_pre_matter, format_and_emit_report, load_config, CliOptions, Color,
-    ErrorKind, FileName, Input, Summary, Verbosity, WriteMode,
+    Config, ErrorKind, FileLines, FileName, Input, Summary, Verbosity, WriteMode,
 };
 
 fn main() {
@@ -172,7 +174,7 @@ fn is_nightly() -> bool {
 
 fn execute(opts: &Options) -> Result<(WriteMode, Summary), failure::Error> {
     let matches = opts.parse(env::args().skip(1))?;
-    let options = CliOptions::from_matches(&matches)?;
+    let options = GetOptsOptions::from_matches(&matches)?;
 
     match determine_operation(&matches)? {
         Operation::Help(HelpOp::None) => {
@@ -203,7 +205,7 @@ fn execute(opts: &Options) -> Result<(WriteMode, Summary), failure::Error> {
         }
         Operation::Stdin { input } => {
             // try to read config from local directory
-            let (mut config, _) = load_config(Some(Path::new(".")), Some(&options))?;
+            let (mut config, _) = load_config(Some(Path::new(".")), Some(options.clone()))?;
 
             // write_mode is always Display for Stdin.
             config.set().write_mode(WriteMode::Display);
@@ -238,10 +240,10 @@ fn execute(opts: &Options) -> Result<(WriteMode, Summary), failure::Error> {
 fn format(
     files: Vec<PathBuf>,
     minimal_config_path: Option<String>,
-    options: CliOptions,
+    options: GetOptsOptions,
 ) -> Result<(WriteMode, Summary), failure::Error> {
     options.verify_file_lines(&files);
-    let (config, config_path) = load_config(None, Some(&options))?;
+    let (config, config_path) = load_config(None, Some(options.clone()))?;
 
     if config.verbose() == Verbosity::Verbose {
         if let Some(path) = config_path.as_ref() {
@@ -263,7 +265,7 @@ fn format(
             // Check the file directory if the config-path could not be read or not provided
             let local_config = if config_path.is_none() {
                 let (local_config, config_path) =
-                    load_config(Some(file.parent().unwrap()), Some(&options))?;
+                    load_config(Some(file.parent().unwrap()), Some(options.clone()))?;
                 if local_config.verbose() == Verbosity::Verbose {
                     if let Some(path) = config_path {
                         println!(
@@ -403,3 +405,145 @@ fn determine_operation(matches: &Matches) -> Result<Operation, ErrorKind> {
         minimal_config_path,
     })
 }
+
+const STABLE_WRITE_MODES: [WriteMode; 4] = [
+    WriteMode::Replace,
+    WriteMode::Overwrite,
+    WriteMode::Display,
+    WriteMode::Check,
+];
+
+/// Parsed command line options.
+#[derive(Clone, Debug, Default)]
+struct GetOptsOptions {
+    skip_children: Option<bool>,
+    quiet: bool,
+    verbose: bool,
+    config_path: Option<PathBuf>,
+    write_mode: WriteMode,
+    check: bool,
+    color: Option<Color>,
+    file_lines: FileLines, // Default is all lines in all files.
+    unstable_features: bool,
+    error_on_unformatted: Option<bool>,
+}
+
+impl GetOptsOptions {
+    pub fn from_matches(matches: &Matches) -> Result<GetOptsOptions, failure::Error> {
+        let mut options = GetOptsOptions::default();
+        options.verbose = matches.opt_present("verbose");
+        options.quiet = matches.opt_present("quiet");
+        if options.verbose && options.quiet {
+            return Err(format_err!("Can't use both `--verbose` and `--quiet`"));
+        }
+
+        let rust_nightly = option_env!("CFG_RELEASE_CHANNEL")
+            .map(|c| c == "nightly")
+            .unwrap_or(false);
+        if rust_nightly {
+            options.unstable_features = matches.opt_present("unstable-features");
+        }
+
+        if options.unstable_features {
+            if matches.opt_present("skip-children") {
+                options.skip_children = Some(true);
+            }
+            if matches.opt_present("error-on-unformatted") {
+                options.error_on_unformatted = Some(true);
+            }
+            if let Some(ref file_lines) = matches.opt_str("file-lines") {
+                options.file_lines = file_lines.parse().map_err(err_msg)?;
+            }
+        }
+
+        options.config_path = matches.opt_str("config-path").map(PathBuf::from);
+
+        options.check = matches.opt_present("check");
+        if let Some(ref emit_str) = matches.opt_str("emit") {
+            if options.check {
+                return Err(format_err!("Invalid to use `--emit` and `--check`"));
+            }
+            if let Ok(write_mode) = write_mode_from_emit_str(emit_str) {
+                options.write_mode = write_mode;
+            } else {
+                return Err(format_err!("Invalid value for `--emit`"));
+            }
+        }
+
+        if options.write_mode == WriteMode::Overwrite && matches.opt_present("backup") {
+            options.write_mode = WriteMode::Replace;
+        }
+
+        if !rust_nightly {
+            if !STABLE_WRITE_MODES.contains(&options.write_mode) {
+                return Err(format_err!(
+                    "Invalid value for `--emit` - using an unstable \
+                     value without `--unstable-features`",
+                ));
+            }
+        }
+
+        if let Some(ref color) = matches.opt_str("color") {
+            match Color::from_str(color) {
+                Ok(color) => options.color = Some(color),
+                _ => return Err(format_err!("Invalid color: {}", color)),
+            }
+        }
+
+        Ok(options)
+    }
+
+    fn verify_file_lines(&self, files: &[PathBuf]) {
+        for f in self.file_lines.files() {
+            match *f {
+                FileName::Real(ref f) if files.contains(f) => {}
+                FileName::Real(_) => {
+                    eprintln!("Warning: Extra file listed in file_lines option '{}'", f)
+                }
+                _ => eprintln!("Warning: Not a file '{}'", f),
+            }
+        }
+    }
+}
+
+impl CliOptions for GetOptsOptions {
+    fn apply_to(self, config: &mut Config) {
+        if self.verbose {
+            config.set().verbose(Verbosity::Verbose);
+        } else if self.quiet {
+            config.set().verbose(Verbosity::Quiet);
+        } else {
+            config.set().verbose(Verbosity::Normal);
+        }
+        config.set().file_lines(self.file_lines);
+        config.set().unstable_features(self.unstable_features);
+        if let Some(skip_children) = self.skip_children {
+            config.set().skip_children(skip_children);
+        }
+        if let Some(error_on_unformatted) = self.error_on_unformatted {
+            config.set().error_on_unformatted(error_on_unformatted);
+        }
+        if self.check {
+            config.set().write_mode(WriteMode::Check);
+        } else {
+            config.set().write_mode(self.write_mode);
+        }
+        if let Some(color) = self.color {
+            config.set().color(color);
+        }
+    }
+
+    fn config_path(&self) -> Option<&Path> {
+        self.config_path.as_ref().map(|p| &**p)
+    }
+}
+
+fn write_mode_from_emit_str(emit_str: &str) -> Result<WriteMode, failure::Error> {
+    match emit_str {
+        "files" => Ok(WriteMode::Overwrite),
+        "stdout" => Ok(WriteMode::Display),
+        "coverage" => Ok(WriteMode::Coverage),
+        "checkstyle" => Ok(WriteMode::Checkstyle),
+        _ => Err(format_err!("Invalid value for `--emit`")),
+    }
+}

src/config/mod.rs

@@ -17,7 +17,7 @@
 use std::{env, fs};
 
 use config::config_type::ConfigType;
-use config::file_lines::FileLines;
+pub use config::file_lines::FileLines;
 pub use config::lists::*;
 pub use config::options::*;
 
@@ -146,12 +146,12 @@
         "'small' heuristic values";
 }
 
-pub fn load_config(
+pub fn load_config<O: CliOptions>(
     file_path: Option<&Path>,
-    options: Option<&CliOptions>,
+    options: Option<O>,
 ) -> Result<(Config, Option<PathBuf>), Error> {
     let over_ride = match options {
-        Some(opts) => config_path(opts)?,
+        Some(ref opts) => config_path(opts)?,
         None => None,
     };
 
@@ -165,7 +165,7 @@ pub fn load_config(
 
     result.map(|(mut c, p)| {
         if let Some(options) = options {
-            options.clone().apply_to(&mut c);
+            options.apply_to(&mut c);
         }
         (c, p)
     })
@@ -208,9 +208,9 @@ fn config_path(options: &CliOptions) -> Result<Option<PathBuf>, Error> {
 
     // Read the config_path and convert to parent dir if a file is provided.
     // If a config file cannot be found from the given path, return error.
-    match options.config_path {
+    match options.config_path() {
-        Some(ref path) if !path.exists() => config_path_not_found(path.to_str().unwrap()),
+        Some(path) if !path.exists() => config_path_not_found(path.to_str().unwrap()),
-        Some(ref path) if path.is_dir() => {
+        Some(path) if path.is_dir() => {
             let config_file_path = get_toml_path(path)?;
             if config_file_path.is_some() {
                 Ok(config_file_path)
@@ -218,7 +218,7 @@ fn config_path(options: &CliOptions) -> Result<Option<PathBuf>, Error> {
                 config_path_not_found(path.to_str().unwrap())
             }
         }
-        ref path => Ok(path.to_owned()),
+        path => Ok(path.map(|p| p.to_owned())),
     }
 }
 

src/config/options.rs

@@ -11,16 +11,11 @@
 use syntax::codemap::FileName;
 
 use config::config_type::ConfigType;
-use config::file_lines::FileLines;
 use config::lists::*;
 use config::Config;
 
-use failure::{self, err_msg};
-
-use getopts::Matches;
 use std::collections::HashSet;
 use std::path::{Path, PathBuf};
-use std::str::FromStr;
 
 /// Macro for deriving implementations of Serialize/Deserialize for enums
 #[macro_export]
@@ -193,13 +188,6 @@ pub fn to_list_tactic(self) -> ListTactic {
     None,
 }
 
-const STABLE_WRITE_MODES: [WriteMode; 4] = [
-    WriteMode::Replace,
-    WriteMode::Overwrite,
-    WriteMode::Display,
-    WriteMode::Check,
-];
-
 configuration_option_enum! { Color:
     // Always use color, whether it is a piped or terminal output
     Always,
@@ -328,131 +316,7 @@ fn from_str(_: &str) -> Result<Self, Self::Err> {
     }
 }
 
-/// Parsed command line options.
+pub trait CliOptions {
-#[derive(Clone, Debug, Default)]
+    fn apply_to(self, config: &mut Config);
-pub struct CliOptions {
+    fn config_path(&self) -> Option<&Path>;
-    pub skip_children: Option<bool>,
-    pub quiet: bool,
-    pub verbose: bool,
-    pub config_path: Option<PathBuf>,
-    pub write_mode: WriteMode,
-    pub check: bool,
-    pub color: Option<Color>,
-    pub file_lines: FileLines, // Default is all lines in all files.
-    pub unstable_features: bool,
-    pub error_on_unformatted: Option<bool>,
-}
-
-impl CliOptions {
-    pub fn from_matches(matches: &Matches) -> Result<CliOptions, failure::Error> {
-        let mut options = CliOptions::default();
-        options.verbose = matches.opt_present("verbose");
-        options.quiet = matches.opt_present("quiet");
-        if options.verbose && options.quiet {
-            return Err(format_err!("Can't use both `--verbose` and `--quiet`"));
-        }
-
-        let rust_nightly = option_env!("CFG_RELEASE_CHANNEL")
-            .map(|c| c == "nightly")
-            .unwrap_or(false);
-        if rust_nightly {
-            options.unstable_features = matches.opt_present("unstable-features");
-        }
-
-        if options.unstable_features {
-            if matches.opt_present("skip-children") {
-                options.skip_children = Some(true);
-            }
-            if matches.opt_present("error-on-unformatted") {
-                options.error_on_unformatted = Some(true);
-            }
-            if let Some(ref file_lines) = matches.opt_str("file-lines") {
-                options.file_lines = file_lines.parse().map_err(err_msg)?;
-            }
-        }
-
-        options.config_path = matches.opt_str("config-path").map(PathBuf::from);
-
-        options.check = matches.opt_present("check");
-        if let Some(ref emit_str) = matches.opt_str("emit") {
-            if options.check {
-                return Err(format_err!("Invalid to use `--emit` and `--check`"));
-            }
-            if let Ok(write_mode) = write_mode_from_emit_str(emit_str) {
-                options.write_mode = write_mode;
-            } else {
-                return Err(format_err!("Invalid value for `--emit`"));
-            }
-        }
-
-        if options.write_mode == WriteMode::Overwrite && matches.opt_present("backup") {
-            options.write_mode = WriteMode::Replace;
-        }
-
-        if !rust_nightly {
-            if !STABLE_WRITE_MODES.contains(&options.write_mode) {
-                return Err(format_err!(
-                    "Invalid value for `--emit` - using an unstable \
-                     value without `--unstable-features`",
-                ));
-            }
-        }
-
-        if let Some(ref color) = matches.opt_str("color") {
-            match Color::from_str(color) {
-                Ok(color) => options.color = Some(color),
-                _ => return Err(format_err!("Invalid color: {}", color)),
-            }
-        }
-
-        Ok(options)
-    }
-
-    pub fn apply_to(self, config: &mut Config) {
-        if self.verbose {
-            config.set().verbose(Verbosity::Verbose);
-        } else if self.quiet {
-            config.set().verbose(Verbosity::Quiet);
-        } else {
-            config.set().verbose(Verbosity::Normal);
-        }
-        config.set().file_lines(self.file_lines);
-        config.set().unstable_features(self.unstable_features);
-        if let Some(skip_children) = self.skip_children {
-            config.set().skip_children(skip_children);
-        }
-        if let Some(error_on_unformatted) = self.error_on_unformatted {
-            config.set().error_on_unformatted(error_on_unformatted);
-        }
-        if self.check {
-            config.set().write_mode(WriteMode::Check);
-        } else {
-            config.set().write_mode(self.write_mode);
-        }
-        if let Some(color) = self.color {
-            config.set().color(color);
-        }
-    }
-
-    pub fn verify_file_lines(&self, files: &[PathBuf]) {
-        for f in self.file_lines.files() {
-            match *f {
-                FileName::Real(ref f) if files.contains(f) => {}
-                FileName::Real(_) => {
-                    eprintln!("Warning: Extra file listed in file_lines option '{}'", f)
-                }
-                _ => eprintln!("Warning: Not a file '{}'", f),
-            }
-        }
-    }
-}
-
-fn write_mode_from_emit_str(emit_str: &str) -> Result<WriteMode, failure::Error> {
-    match emit_str {
-        "files" => Ok(WriteMode::Overwrite),
-        "stdout" => Ok(WriteMode::Display),
-        "coverage" => Ok(WriteMode::Coverage),
-        "checkstyle" => Ok(WriteMode::Checkstyle),
-        _ => Err(format_err!("Invalid value for `--emit`")),
-    }
 }

src/git-rustfmt/main.rs

@@ -21,7 +21,7 @@
 
 use getopts::{Matches, Options};
 
-use rustfmt::{format_and_emit_report, load_config, Input};
+use rustfmt::{format_and_emit_report, load_config, CliOptions, Input};
 
 fn prune_files(files: Vec<&str>) -> Vec<&str> {
     let prefixes: Vec<_> = files
@@ -68,7 +68,8 @@ fn get_files(input: &str) -> Vec<&str> {
 }
 
 fn fmt_files(files: &[&str]) -> i32 {
-    let (config, _) = load_config(Some(Path::new(".")), None).expect("couldn't load config");
+    let (config, _) =
+        load_config::<NullOptions>(Some(Path::new(".")), None).expect("couldn't load config");
 
     let mut exit_code = 0;
     for file in files {
@@ -80,6 +81,17 @@ fn fmt_files(files: &[&str]) -> i32 {
     exit_code
 }
 
+struct NullOptions;
+
+impl CliOptions for NullOptions {
+    fn apply_to(self, _: &mut rustfmt::Config) {
+        unreachable!();
+    }
+    fn config_path(&self) -> Option<&Path> {
+        unreachable!();
+    }
+}
+
 fn uncommitted_files() -> Vec<String> {
     let mut cmd = Command::new("git");
     cmd.arg("ls-files");

src/lib.rs

@@ -19,7 +19,6 @@
 extern crate diff;
 #[macro_use]
 extern crate failure;
-extern crate getopts;
 extern crate itertools;
 #[cfg(test)]
 #[macro_use]
@@ -62,7 +61,7 @@
 
 pub use config::options::CliOptions;
 pub use config::summary::Summary;
-pub use config::{file_lines, load_config, Config, Verbosity, WriteMode};
+pub use config::{load_config, Color, Config, FileLines, Verbosity, WriteMode};
 
 #[macro_use]
 mod utils;

src/test/mod.rs

@@ -380,12 +380,12 @@ fn format_file<P: Into<PathBuf>>(filepath: P, config: &Config) -> (Summary, File
     syntax::with_globals(|| format_input_inner::<io::Stdout>(input, config, None)).unwrap()
 }
 
-pub enum IdempotentCheckError {
+enum IdempotentCheckError {
     Mismatch(HashMap<PathBuf, Vec<Mismatch>>),
     Parse,
 }
 
-pub fn idempotent_check(
+fn idempotent_check(
     filename: &PathBuf,
     opt_config: &Option<PathBuf>,
 ) -> Result<FormatReport, IdempotentCheckError> {