From 14dbac5fd78a76592bcf4b9fbb71da3ba102e1ec Mon Sep 17 00:00:00 2001 From: Kamal Marhubi Date: Fri, 5 Feb 2016 15:59:41 -0500 Subject: [PATCH] config: Use write_mode from config This commit tidies up handling of `write_mode` by setting it in the config at the start, and removing the `write_mode` parameter threaded throughout the formatting process. --- src/bin/rustfmt.rs | 52 +++++++++++++++++++++++++-------------------- src/config.rs | 5 +---- src/expr.rs | 6 ++---- src/filemap.rs | 18 +++++----------- src/items.rs | 2 +- src/lib.rs | 41 ++++++++++++----------------------- src/missed_spans.rs | 11 +++------- src/visitor.rs | 9 ++------ tests/system.rs | 39 ++++++++++++++++++++-------------- 9 files changed, 79 insertions(+), 104 deletions(-) diff --git a/src/bin/rustfmt.rs b/src/bin/rustfmt.rs index fed74868f6a..8a393ba399d 100644 --- a/src/bin/rustfmt.rs +++ b/src/bin/rustfmt.rs @@ -24,13 +24,14 @@ use std::env; 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}; /// Rustfmt operations. enum Operation { /// Format files and their child modules. - Format(Vec, WriteMode, Option), + Format(Vec, Option), /// Print the help message. Help, // Print version information @@ -40,7 +41,7 @@ enum Operation { /// Invalid program input, including reason. InvalidInput(String), /// No file specified, read from stdin - Stdin(String, WriteMode, Option), + Stdin(String, Option), } /// Try to find a project file in the given directory and its parents. Returns the path of a the @@ -109,9 +110,19 @@ fn match_cli_path_or_file(config_path: Option, resolve_config(input_file) } -fn update_config(config: &mut Config, matches: &Matches) { +fn update_config(config: &mut Config, matches: &Matches) -> Result<(), String> { config.verbose = matches.opt_present("verbose"); config.skip_children = matches.opt_present("skip-children"); + + let write_mode = matches.opt_str("write-mode"); + match matches.opt_str("write-mode").map(|wm| WriteMode::from_str(&wm)) { + None => Ok(()), + Some(Ok(write_mode)) => { + config.write_mode = write_mode; + Ok(()) + } + Some(Err(_)) => Err(format!("Invalid write-mode: {}", write_mode.expect("cannot happen"))), + } } fn execute() -> i32 { @@ -161,15 +172,18 @@ fn execute() -> i32 { Config::print_docs(); 0 } - Operation::Stdin(input, write_mode, config_path) => { + Operation::Stdin(input, config_path) => { // try to read config from local directory - let (config, _) = match_cli_path_or_file(config_path, &env::current_dir().unwrap()) - .expect("Error resolving config"); + let (mut config, _) = match_cli_path_or_file(config_path, &env::current_dir().unwrap()) + .expect("Error resolving config"); - run_from_stdin(input, write_mode, &config); + // write_mode is always Plain for Stdin. + config.write_mode = WriteMode::Plain; + + run_from_stdin(input, &config); 0 } - Operation::Format(files, write_mode, config_path) => { + Operation::Format(files, config_path) => { let mut config = Config::default(); let mut path = None; // Load the config path file if provided @@ -198,8 +212,11 @@ fn execute() -> i32 { config = config_tmp; } - update_config(&mut config, &matches); - run(&file, write_mode, &config); + if let Err(e) = update_config(&mut config, &matches) { + print_usage(&opts, &e); + return 1; + } + run(&file, &config); } 0 } @@ -267,21 +284,10 @@ fn determine_operation(matches: &Matches) -> Operation { Err(e) => return Operation::InvalidInput(e.to_string()), } - // WriteMode is always plain for Stdin - return Operation::Stdin(buffer, WriteMode::Plain, config_path); + return Operation::Stdin(buffer, config_path); } - let write_mode = match matches.opt_str("write-mode") { - Some(mode) => { - match mode.parse() { - Ok(mode) => mode, - Err(..) => return Operation::InvalidInput("Unrecognized write mode".into()), - } - } - None => WriteMode::Default, - }; - let files: Vec<_> = matches.free.iter().map(PathBuf::from).collect(); - Operation::Format(files, write_mode, config_path) + Operation::Format(files, config_path) } diff --git a/src/config.rs b/src/config.rs index 59b301eebf1..da686ffac95 100644 --- a/src/config.rs +++ b/src/config.rs @@ -121,9 +121,6 @@ configuration_option_enum! { ReportTactic: } configuration_option_enum! { WriteMode: - // Used internally to represent when no option is given - // Treated as Replace. - Default, // Backsup the original file and overwrites the orignal. Replace, // Overwrites original file without backup. @@ -349,6 +346,6 @@ create_config! { match_block_trailing_comma: bool, false, "Put a trailing comma after a block based match arm (non-block arms are not affected)"; match_wildcard_trailing_comma: bool, true, "Put a trailing comma after a wildcard arm"; - write_mode: WriteMode, WriteMode::Default, + write_mode: WriteMode, WriteMode::Replace, "What Write Mode to use when none is supplied: Replace, Overwrite, Display, Diff, Coverage"; } diff --git a/src/expr.rs b/src/expr.rs index b299f82a883..34a30e94899 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -427,7 +427,7 @@ impl Rewrite for ast::Block { return Some(user_str); } - let mut visitor = FmtVisitor::from_codemap(context.parse_session, context.config, None); + let mut visitor = FmtVisitor::from_codemap(context.parse_session, context.config); visitor.block_indent = context.block_indent; let prefix = match self.rules { @@ -954,9 +954,7 @@ impl Rewrite for ast::Arm { let attr_str = if !attrs.is_empty() { // We only use this visitor for the attributes, should we use it for // more? - let mut attr_visitor = FmtVisitor::from_codemap(context.parse_session, - context.config, - None); + let mut attr_visitor = FmtVisitor::from_codemap(context.parse_session, context.config); attr_visitor.block_indent = context.block_indent; attr_visitor.last_pos = attrs[0].span.lo; if attr_visitor.visit_attrs(attrs) { diff --git a/src/filemap.rs b/src/filemap.rs index 61ad573f702..f41915b8cff 100644 --- a/src/filemap.rs +++ b/src/filemap.rs @@ -31,18 +31,14 @@ pub fn append_newlines(file_map: &mut FileMap) { } } -pub fn write_all_files(file_map: &FileMap, - mut out: T, - mode: WriteMode, - config: &Config) - -> Result<(), io::Error> +pub fn write_all_files(file_map: &FileMap, mut out: T, config: &Config) -> Result<(), io::Error> where T: Write { - output_header(&mut out, mode).ok(); + output_header(&mut out, config.write_mode).ok(); for filename in file_map.keys() { - try!(write_file(&file_map[filename], filename, &mut out, mode, config)); + try!(write_file(&file_map[filename], filename, &mut out, config)); } - output_footer(&mut out, mode).ok(); + output_footer(&mut out, config.write_mode).ok(); Ok(()) } @@ -87,7 +83,6 @@ pub fn write_system_newlines(writer: T, pub fn write_file(text: &StringBuffer, filename: &str, out: &mut T, - mode: WriteMode, config: &Config) -> Result, io::Error> where T: Write @@ -114,7 +109,7 @@ pub fn write_file(text: &StringBuffer, Ok(make_diff(&ori, &fmt, 3)) } - match mode { + match config.write_mode { WriteMode::Replace => { if let Ok((ori, fmt)) = source_and_formatted_text(text, filename, config) { if fmt != ori { @@ -157,9 +152,6 @@ pub fn write_file(text: &StringBuffer, |line_num| format!("\nDiff at line {}:", line_num)); } } - WriteMode::Default => { - unreachable!("The WriteMode should NEVER Be default at this point!"); - } WriteMode::Checkstyle => { let diff = try!(create_diff(filename, text, config)); try!(output_checkstyle_file(out, filename, diff)); diff --git a/src/items.rs b/src/items.rs index 2ff52148f6a..d7c16c4561c 100644 --- a/src/items.rs +++ b/src/items.rs @@ -530,7 +530,7 @@ pub fn format_impl(context: &RewriteContext, item: &ast::Item, offset: Indent) - let open_pos = try_opt!(snippet.find_uncommented("{")) + 1; if !items.is_empty() || contains_comment(&snippet[open_pos..]) { - let mut visitor = FmtVisitor::from_codemap(context.parse_session, context.config, None); + let mut visitor = FmtVisitor::from_codemap(context.parse_session, context.config); visitor.block_indent = context.block_indent.block_indent(context.config); visitor.last_pos = item.span.lo + BytePos(open_pos as u32); diff --git a/src/lib.rs b/src/lib.rs index 2f49ed4f5e5..ee671cf0baa 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -39,7 +39,7 @@ use std::fmt; use issues::{BadIssueSeeker, Issue}; use filemap::FileMap; use visitor::FmtVisitor; -use config::{Config, WriteMode}; +use config::Config; #[macro_use] mod utils; @@ -264,8 +264,7 @@ impl fmt::Display for FormatReport { fn fmt_ast(krate: &ast::Crate, parse_session: &ParseSess, main_file: &Path, - config: &Config, - mode: WriteMode) + config: &Config) -> FileMap { let mut file_map = FileMap::new(); for (path, module) in modules::list_files(krate, parse_session.codemap()) { @@ -276,7 +275,7 @@ fn fmt_ast(krate: &ast::Crate, if config.verbose { println!("Formatting {}", path); } - let mut visitor = FmtVisitor::from_codemap(parse_session, config, Some(mode)); + let mut visitor = FmtVisitor::from_codemap(parse_session, config); visitor.format_separate_mod(module); file_map.insert(path.to_owned(), visitor.buffer); } @@ -366,7 +365,7 @@ pub fn fmt_lines(file_map: &mut FileMap, config: &Config) -> FormatReport { report } -pub fn format_string(input: String, config: &Config, mode: WriteMode) -> FileMap { +pub fn format_string(input: String, config: &Config) -> FileMap { let path = "stdin"; let mut parse_session = ParseSess::new(); let krate = parse::parse_crate_from_source_str(path.to_owned(), @@ -383,7 +382,7 @@ pub fn format_string(input: String, config: &Config, mode: WriteMode) -> FileMap let mut file_map = FileMap::new(); // do the actual formatting - let mut visitor = FmtVisitor::from_codemap(&parse_session, config, Some(mode)); + let mut visitor = FmtVisitor::from_codemap(&parse_session, config); visitor.format_separate_mod(&krate.module); // append final newline @@ -393,7 +392,7 @@ pub fn format_string(input: String, config: &Config, mode: WriteMode) -> FileMap file_map } -pub fn format(file: &Path, config: &Config, mode: WriteMode) -> FileMap { +pub fn format(file: &Path, config: &Config) -> FileMap { let mut parse_session = ParseSess::new(); let krate = parse::parse_crate_from_file(file, Vec::new(), &parse_session); @@ -401,7 +400,7 @@ pub fn format(file: &Path, config: &Config, mode: WriteMode) -> FileMap { let emitter = Box::new(EmitterWriter::new(Box::new(Vec::new()), None)); parse_session.span_diagnostic.handler = Handler::with_emitter(false, emitter); - let mut file_map = fmt_ast(&krate, &parse_session, file, config, mode); + let mut file_map = fmt_ast(&krate, &parse_session, file, config); // For some reason, the codemap does not include terminating // newlines so we must add one on for each file. This is sad. @@ -410,25 +409,12 @@ pub fn format(file: &Path, config: &Config, mode: WriteMode) -> FileMap { file_map } -// Make sure that we are using the correct WriteMode, -// preferring what is passed as an argument -fn check_write_mode(arg: WriteMode, config: WriteMode) -> WriteMode { - match (arg, config) { - (WriteMode::Default, WriteMode::Default) => WriteMode::Replace, - (WriteMode::Default, mode) => mode, - (mode, _) => mode, - } -} - -// write_mode determines what happens to the result of running rustfmt, see -// WriteMode. -pub fn run(file: &Path, write_mode: WriteMode, config: &Config) { - let mode = check_write_mode(write_mode, config.write_mode); - let mut result = format(file, config, mode); +pub fn run(file: &Path, config: &Config) { + let mut result = format(file, config); print!("{}", fmt_lines(&mut result, config)); let out = stdout(); - let write_result = filemap::write_all_files(&result, out, mode, config); + let write_result = filemap::write_all_files(&result, out, config); if let Err(msg) = write_result { println!("Error writing files: {}", msg); @@ -436,13 +422,12 @@ pub fn run(file: &Path, write_mode: WriteMode, config: &Config) { } // Similar to run, but takes an input String instead of a file to format -pub fn run_from_stdin(input: String, write_mode: WriteMode, config: &Config) { - let mode = check_write_mode(write_mode, config.write_mode); - let mut result = format_string(input, config, mode); +pub fn run_from_stdin(input: String, config: &Config) { + let mut result = format_string(input, config); fmt_lines(&mut result, config); let mut out = stdout(); - let write_result = filemap::write_file(&result["stdin"], "stdin", &mut out, mode, config); + let write_result = filemap::write_file(&result["stdin"], "stdin", &mut out, config); if let Err(msg) = write_result { panic!("Error writing to stdout: {}", msg); diff --git a/src/missed_spans.rs b/src/missed_spans.rs index c86cd62a900..092cc0093f1 100644 --- a/src/missed_spans.rs +++ b/src/missed_spans.rs @@ -103,14 +103,9 @@ impl<'a> FmtVisitor<'a> { .collect() } - let replaced = match self.write_mode { - Some(mode) => { - match mode { - WriteMode::Coverage => replace_chars(old_snippet), - _ => old_snippet.to_owned(), - } - } - None => old_snippet.to_owned(), + let replaced = match self.config.write_mode { + WriteMode::Coverage => replace_chars(old_snippet), + _ => old_snippet.to_owned(), }; let snippet = &*replaced; diff --git a/src/visitor.rs b/src/visitor.rs index 073a05ef2bc..f53ed2246d5 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -17,7 +17,7 @@ use strings::string_buffer::StringBuffer; use Indent; use utils; -use config::{Config, WriteMode}; +use config::Config; use rewrite::{Rewrite, RewriteContext}; use comment::rewrite_comment; use macros::rewrite_macro; @@ -31,7 +31,6 @@ pub struct FmtVisitor<'a> { // FIXME: use an RAII util or closure for indenting pub block_indent: Indent, pub config: &'a Config, - pub write_mode: Option, } impl<'a> FmtVisitor<'a> { @@ -380,10 +379,7 @@ impl<'a> FmtVisitor<'a> { self.last_pos = span.hi; } - pub fn from_codemap(parse_session: &'a ParseSess, - config: &'a Config, - mode: Option) - -> FmtVisitor<'a> { + pub fn from_codemap(parse_session: &'a ParseSess, config: &'a Config) -> FmtVisitor<'a> { FmtVisitor { parse_session: parse_session, codemap: parse_session.codemap(), @@ -394,7 +390,6 @@ impl<'a> FmtVisitor<'a> { alignment: 0, }, config: config, - write_mode: mode, } } diff --git a/tests/system.rs b/tests/system.rs index 8c7ea6c5fe5..497583f9d67 100644 --- a/tests/system.rs +++ b/tests/system.rs @@ -44,7 +44,7 @@ fn system_tests() { // Turn a DirEntry into a String that represents the relative path to the // file. let files = files.map(get_path_string); - let (_reports, count, fails) = check_files(files, WriteMode::Default); + let (_reports, count, fails) = check_files(files, None); // Display results. println!("Ran {} system tests.", count); @@ -57,7 +57,7 @@ fn system_tests() { fn coverage_tests() { let files = fs::read_dir("tests/coverage-source").expect("Couldn't read source dir"); let files = files.map(get_path_string); - let (_reports, count, fails) = check_files(files, WriteMode::Coverage); + let (_reports, count, fails) = check_files(files, Some(WriteMode::Coverage)); println!("Ran {} tests in coverage mode.", count); assert!(fails == 0, "{} tests failed", fails); @@ -67,19 +67,23 @@ fn coverage_tests() { fn checkstyle_test() { let filename = "tests/source/fn-single-line.rs"; let expected_filename = "tests/writemode/checkstyle.xml"; - assert_output(filename, expected_filename, WriteMode::Checkstyle); + assert_output(filename, expected_filename, Some(WriteMode::Checkstyle)); } // Helper function for comparing the results of rustfmt // to a known output file generated by one of the write modes. -fn assert_output(source: &str, expected_filename: &str, write_mode: WriteMode) { - let config = read_config(&source); +fn assert_output(source: &str, expected_filename: &str, write_mode: Option) { let file_map = run_rustfmt(source.to_string(), write_mode); + let mut config = read_config(&source); + if let Some(write_mode) = write_mode { + config.write_mode = write_mode; + } + // Populate output by writing to a vec. let mut out = vec![]; - let _ = filemap::write_all_files(&file_map, &mut out, write_mode, &config); + let _ = filemap::write_all_files(&file_map, &mut out, &config); let output = String::from_utf8(out).unwrap(); let mut expected_file = fs::File::open(&expected_filename).expect("Couldn't open target"); @@ -104,7 +108,7 @@ fn idempotence_tests() { let files = fs::read_dir("tests/target") .expect("Couldn't read target dir") .map(get_path_string); - let (_reports, count, fails) = check_files(files, WriteMode::Default); + let (_reports, count, fails) = check_files(files, None); // Display results. println!("Ran {} idempotent tests.", count); @@ -122,7 +126,7 @@ fn self_tests() { // Hack because there's no `IntoIterator` impl for `[T; N]`. let files = files.chain(Some("src/lib.rs".to_owned()).into_iter()); - let (reports, count, fails) = check_files(files, WriteMode::Default); + let (reports, count, fails) = check_files(files, None); let mut warnings = 0; // Display results. @@ -141,7 +145,7 @@ fn self_tests() { // For each file, run rustfmt and collect the output. // Returns the number of files checked and the number of failures. -fn check_files(files: I, write_mode: WriteMode) -> (Vec, u32, u32) +fn check_files(files: I, write_mode: Option) -> (Vec, u32, u32) where I: Iterator { let mut count = 0; @@ -192,13 +196,16 @@ fn read_config(filename: &str) -> Config { } // Simulate run() -fn run_rustfmt(filename: String, write_mode: WriteMode) -> FileMap { - let config = read_config(&filename); - format(Path::new(&filename), &config, write_mode) +fn run_rustfmt(filename: String, write_mode: Option) -> FileMap { + let mut config = read_config(&filename); + if let Some(write_mode) = write_mode { + config.write_mode = write_mode; + } + format(Path::new(&filename), &config) } pub fn idempotent_check(filename: String, - write_mode: WriteMode) + write_mode: Option) -> Result>> { let sig_comments = read_significant_comments(&filename); let config = read_config(&filename); @@ -266,7 +273,7 @@ fn read_significant_comments(file_name: &str) -> HashMap { // TODO: needs a better name, more explanation. fn handle_result(result: HashMap, target: Option<&str>, - write_mode: WriteMode) + write_mode: Option) -> Result<(), HashMap>> { let mut failures = HashMap::new(); @@ -292,10 +299,10 @@ fn handle_result(result: HashMap, } // Map source file paths to their target paths. -fn get_target(file_name: &str, target: Option<&str>, write_mode: WriteMode) -> String { +fn get_target(file_name: &str, target: Option<&str>, write_mode: Option) -> String { let file_path = Path::new(file_name); let (source_path_prefix, target_path_prefix) = match write_mode { - WriteMode::Coverage => { + Some(WriteMode::Coverage) => { (Path::new("tests/coverage-source/"), "tests/coverage-target/") }