From bc8d6d20a9905376a036163f327b6035f721e20c Mon Sep 17 00:00:00 2001 From: Bastian Schubert Date: Tue, 1 Nov 2022 07:07:49 +0100 Subject: [PATCH] apply review comments --- src/cobertura.rs | 18 +++-------- src/lib.rs | 3 ++ src/main.rs | 59 +++++++++++++++++++++++++---------- src/output.rs | 71 ++++++++++--------------------------------- src/path_rewriting.rs | 3 +- 5 files changed, 69 insertions(+), 85 deletions(-) diff --git a/src/cobertura.rs b/src/cobertura.rs index f97747733..99e188fbe 100644 --- a/src/cobertura.rs +++ b/src/cobertura.rs @@ -7,12 +7,11 @@ use std::path::Path; use std::time::{SystemTime, UNIX_EPOCH}; use std::{ io::{BufWriter, Cursor, Write}, - path::PathBuf, }; use symbolic_common::Name; use symbolic_demangle::{Demangle, DemangleOptions}; -use crate::{output::get_target_output_writable, CovResult}; +use crate::{output::get_target_output_writable, ResultTuple}; macro_rules! demangle { ($name: expr, $demangle: expr, $options: expr) => {{ @@ -231,7 +230,7 @@ impl ToString for ConditionType { } fn get_coverage( - results: &[(PathBuf, PathBuf, CovResult)], + results: &[ResultTuple], sources: Vec, demangle: bool, demangle_options: DemangleOptions, @@ -329,8 +328,8 @@ fn get_coverage( pub fn output_cobertura( source_dir: Option<&Path>, - results: &[(PathBuf, PathBuf, CovResult)], - output_path: Option<&Path>, + results: &[ResultTuple], + output_file: Option<&Path>, demangle: bool, ) { let demangle_options = DemangleOptions::name_only(); @@ -491,14 +490,7 @@ pub fn output_cobertura( .unwrap(); let result = writer.into_inner().into_inner(); - let output_file = output_path.map(|path| { - if path.is_dir() { - path.join("cobertura.xml") - } else { - path.to_path_buf() - } - }); - let mut file = BufWriter::new(get_target_output_writable(output_file.as_ref())); + let mut file = BufWriter::new(get_target_output_writable(output_file)); file.write_all(&result).unwrap(); } diff --git a/src/lib.rs b/src/lib.rs index 8499cad76..0e316aa62 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -44,12 +44,15 @@ pub use crate::file_filter::*; use log::{error, warn}; use std::fs; use std::io::{BufReader, Cursor}; +use std::path::PathBuf; use std::{ collections::{btree_map, hash_map}, path::Path, }; use walkdir::WalkDir; +pub type ResultTuple = (PathBuf, PathBuf, CovResult); + // Merge results, without caring about duplicate lines (they will be removed at the end). pub fn merge_results(result: &mut CovResult, result2: CovResult) -> bool { let mut warn_overflow = false; diff --git a/src/main.rs b/src/main.rs index f02ea170d..16ecfd109 100644 --- a/src/main.rs +++ b/src/main.rs @@ -50,6 +50,27 @@ impl FromStr for OutputType { } } +impl OutputType { + fn to_file_name(&self, output_path: Option<&Path>) -> Option { + output_path.map(|path| { + if path.is_dir() { + match self { + OutputType::Ade => path.join("activedata"), + OutputType::Lcov => path.join("lcov"), + OutputType::Coveralls => path.join("coveralls"), + OutputType::CoverallsPlus => path.join("coveralls+"), + OutputType::Files => path.join("files"), + OutputType::Covdir => path.join("covdir"), + OutputType::Html => path.join("html"), + OutputType::Cobertura => path.join("cobertura.xml"), + } + } else { + path.to_path_buf() + } + }) + } +} + enum Filter { Covered, Uncovered, @@ -108,12 +129,13 @@ struct Opt { use_delimiter = true )] output_types: Vec, - /// Specifies the output path. - #[structopt(short, long, value_name = "PATH", alias = "output-folder")] + /// Specifies the output path. This is a file for a single output type and must be a folder + /// for multiple output types. + #[structopt(short, long, value_name = "PATH", alias = "output-file")] output_path: Option, /// Specifies the output config file. - #[structopt(long, value_name = "PATH", alias = "html-output-config-file")] - html_output_config_file: Option, + #[structopt(long, value_name = "PATH", alias = "output-config-file")] + output_config_file: Option, /// Specifies the root directory of the source files. #[structopt(short, long, value_name = "DIRECTORY", parse(from_os_str))] source_dir: Option, @@ -434,9 +456,11 @@ fn main() { }; for output_type in &opt.output_types { + let output_path = output_type.to_file_name(output_path); + match output_type { - OutputType::Ade => output_activedata_etl(&iterator, output_path, demangle), - OutputType::Lcov => output_lcov(&iterator, output_path, demangle), + OutputType::Ade => output_activedata_etl(&iterator, output_path.as_deref(), demangle), + OutputType::Lcov => output_lcov(&iterator, output_path.as_deref(), demangle), OutputType::Coveralls => output_coveralls( &iterator, opt.token.as_deref(), @@ -446,7 +470,7 @@ fn main() { &service_pull_request, &commit_sha, false, - output_path, + output_path.as_deref(), &opt.vcs_branch, opt.parallel, demangle, @@ -460,24 +484,27 @@ fn main() { &service_pull_request, &commit_sha, true, - output_path, + output_path.as_deref(), &opt.vcs_branch, opt.parallel, demangle, ), - OutputType::Files => output_files(&iterator, output_path), - OutputType::Covdir => output_covdir(&iterator, output_path), + OutputType::Files => output_files(&iterator, output_path.as_deref()), + OutputType::Covdir => output_covdir(&iterator, output_path.as_deref()), OutputType::Html => output_html( &iterator, - output_path, + output_path.as_deref(), num_threads, opt.branch, - opt.html_output_config_file.as_deref(), + opt.output_config_file.as_deref(), + ), + OutputType::Cobertura => output_cobertura( + source_root.as_deref(), + &iterator, + output_path.as_deref(), + demangle, ), - OutputType::Cobertura => { - output_cobertura(source_root.as_deref(), &iterator, output_path, demangle) - }, - OutputType::Markdown => output_markdown(&iterator, opt.output_path.as_deref()), + OutputType::Markdown => output_markdown(&iterator, output_path.as_deref()), }; } } diff --git a/src/output.rs b/src/output.rs index fa4f7dbc0..a02235cf2 100644 --- a/src/output.rs +++ b/src/output.rs @@ -19,7 +19,7 @@ use symbolic_demangle::{Demangle, DemangleOptions}; use tabled::{Style, Table, Tabled}; use uuid::Uuid; -use crate::defs::*; +use crate::{defs::*, ResultTuple}; use crate::html; macro_rules! demangle { @@ -36,7 +36,7 @@ macro_rules! demangle { }}; } -pub fn get_target_output_writable(output_file: Option<&PathBuf>) -> Box { +pub fn get_target_output_writable(output_file: Option<&Path>) -> Box { let write_target: Box = match output_file { Some(output) => { if output.is_dir() { @@ -71,19 +71,12 @@ pub fn get_target_output_writable(output_file: Option<&PathBuf>) -> Box, + results: &[ResultTuple], + output_file: Option<&Path>, demangle: bool, ) { let demangle_options = DemangleOptions::name_only(); - let output_file = output_path.map(|path| { - if path.is_dir() { - path.join("activedata") - } else { - path.to_path_buf() - } - }); - let mut writer = BufWriter::new(get_target_output_writable(output_file.as_ref())); + let mut writer = BufWriter::new(get_target_output_writable(output_file)); for (_, rel_path, result) in results { let covered: Vec = result @@ -191,15 +184,8 @@ pub fn output_activedata_etl( } } -pub fn output_covdir(results: &[(PathBuf, PathBuf, CovResult)], output_path: Option<&Path>) { - let output_file = output_path.map(|path| { - if path.is_dir() { - path.join("covdir") - } else { - path.to_path_buf() - } - }); - let mut writer = BufWriter::new(get_target_output_writable(output_file.as_ref())); +pub fn output_covdir(results: &[ResultTuple], output_file: Option<&Path>) { + let mut writer = BufWriter::new(get_target_output_writable(output_file)); let mut relative: FxHashMap>> = FxHashMap::default(); let global = Rc::new(RefCell::new(CDDirStats::new("".to_string()))); relative.insert(PathBuf::from(""), global.clone()); @@ -255,19 +241,12 @@ pub fn output_covdir(results: &[(PathBuf, PathBuf, CovResult)], output_path: Opt } pub fn output_lcov( - results: &[(PathBuf, PathBuf, CovResult)], - output_path: Option<&Path>, + results: &[ResultTuple], + output_file: Option<&Path>, demangle: bool, ) { let demangle_options = DemangleOptions::name_only(); - let output_file = output_path.map(|path| { - if path.is_dir() { - path.join("lcov") - } else { - path.to_path_buf() - } - }); - let mut writer = BufWriter::new(get_target_output_writable(output_file.as_ref())); + let mut writer = BufWriter::new(get_target_output_writable(output_file)); writer.write_all(b"TN:\n").unwrap(); for (_, rel_path, result) in results { @@ -442,7 +421,7 @@ fn get_coveralls_git_info(commit_sha: &str, vcs_branch: &str) -> Value { } pub fn output_coveralls( - results: &[(PathBuf, PathBuf, CovResult)], + results: &[ResultTuple], repo_token: Option<&str>, service_name: Option<&str>, service_number: &str, @@ -450,7 +429,7 @@ pub fn output_coveralls( service_pull_request: &str, commit_sha: &str, with_function_info: bool, - output_path: Option<&Path>, + output_file: Option<&Path>, vcs_branch: &str, parallel: bool, demangle: bool, @@ -530,36 +509,19 @@ pub fn output_coveralls( obj.insert("service_job_id".to_string(), json!(service_job_id)); } - let output_file = output_path.map(|path| { - if path.is_dir() { - path.join(format!( - "coveralls{}", - if with_function_info { "+" } else { "" } - )) - } else { - path.to_path_buf() - } - }); - let mut writer = BufWriter::new(get_target_output_writable(output_file.as_ref())); + let mut writer = BufWriter::new(get_target_output_writable(output_file)); serde_json::to_writer(&mut writer, &result).unwrap(); } -pub fn output_files(results: &[(PathBuf, PathBuf, CovResult)], output_path: Option<&Path>) { - let output_file = output_path.map(|path| { - if path.is_dir() { - path.join("files") - } else { - path.to_path_buf() - } - }); - let mut writer = BufWriter::new(get_target_output_writable(output_file.as_ref())); +pub fn output_files(results: &[ResultTuple], output_file: Option<&Path>) { + let mut writer = BufWriter::new(get_target_output_writable(output_file)); for (_, rel_path, _) in results { writeln!(writer, "{}", rel_path.display()).unwrap(); } } pub fn output_html( - results: &[(PathBuf, PathBuf, CovResult)], + results: &[ResultTuple], output_dir: Option<&Path>, num_threads: usize, branch_enabled: bool, @@ -570,7 +532,6 @@ pub fn output_html( } else { PathBuf::from("./html") }; - if output.exists() { if !output.is_dir() { eprintln!("{} is not a directory", output.to_str().unwrap()); diff --git a/src/path_rewriting.rs b/src/path_rewriting.rs index 6495e3923..a61fed0f2 100644 --- a/src/path_rewriting.rs +++ b/src/path_rewriting.rs @@ -8,6 +8,7 @@ use std::io; use std::path::{Component, Path, PathBuf}; use walkdir::{DirEntry, WalkDir}; +use crate::ResultTuple; use crate::defs::*; use crate::filter::*; @@ -235,7 +236,7 @@ pub fn rewrite_paths( to_keep_dirs: &[impl AsRef], filter_option: Option, file_filter: crate::FileFilter, -) -> Vec<(PathBuf, PathBuf, CovResult)> { +) -> Vec { let to_ignore_globset = to_globset(to_ignore_dirs); let to_keep_globset = to_globset(to_keep_dirs);