-
-
Notifications
You must be signed in to change notification settings - Fork 383
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JSON Export Reapply #45
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,26 +1,26 @@ | ||
use super::{ExportEntry, ResultExporter}; | ||
|
||
use csv::WriterBuilder; | ||
use std::io::Result; | ||
use std::io::{Error, ErrorKind, Result}; | ||
|
||
pub struct CsvExporter { | ||
out_file: String, | ||
} | ||
pub struct CsvExporter {} | ||
|
||
impl ResultExporter for CsvExporter { | ||
fn write(&self, results: &Vec<ExportEntry>) -> Result<()> { | ||
let mut writer = WriterBuilder::new().from_path(&self.out_file)?; | ||
fn write(&self, results: &Vec<ExportEntry>) -> Result<Vec<u8>> { | ||
let mut writer = WriterBuilder::new().from_writer(vec![]); | ||
for res in results { | ||
writer.serialize(res)?; | ||
} | ||
Ok(()) | ||
|
||
if let Ok(inner) = writer.into_inner() { | ||
return Ok(inner); | ||
} | ||
Err(Error::new(ErrorKind::Other, "Error serializing to CSV")) | ||
} | ||
} | ||
|
||
impl CsvExporter { | ||
pub fn new(file_name: String) -> Self { | ||
CsvExporter { | ||
out_file: file_name, | ||
} | ||
pub fn new() -> Self { | ||
CsvExporter {} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
use super::{ExportEntry, ResultExporter}; | ||
|
||
use std::io::{Error, ErrorKind, Result}; | ||
|
||
use serde_json::to_vec_pretty; | ||
|
||
#[derive(Serialize, Debug)] | ||
struct HyperfineSummary<'a> { | ||
results: &'a Vec<ExportEntry>, | ||
} | ||
|
||
pub struct JsonExporter {} | ||
|
||
impl ResultExporter for JsonExporter { | ||
fn write(&self, results: &Vec<ExportEntry>) -> Result<Vec<u8>> { | ||
let serialized = to_vec_pretty(&HyperfineSummary { results }); | ||
|
||
match serialized { | ||
Ok(file_content) => Ok(file_content), | ||
Err(e) => Err(Error::new(ErrorKind::Other, format!("{:?}", e))), | ||
} | ||
} | ||
} | ||
|
||
impl JsonExporter { | ||
pub fn new() -> Self { | ||
JsonExporter {} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ extern crate serde; | |
|
||
#[macro_use] | ||
extern crate serde_derive; | ||
extern crate serde_json; | ||
extern crate statistical; | ||
|
||
cfg_if! { | ||
|
@@ -38,7 +39,7 @@ mod hyperfine; | |
|
||
use hyperfine::internal::{CmdFailureAction, HyperfineOptions, OutputStyleOption}; | ||
use hyperfine::benchmark::{mean_shell_spawning_time, run_benchmark}; | ||
use hyperfine::export::{create_export_manager, ExportEntry, ResultExportType}; | ||
use hyperfine::export::{create_export_manager, ExportEntry, ExportManager, ResultExportType}; | ||
|
||
/// Print error message to stderr and terminate | ||
pub fn error(message: &str) -> ! { | ||
|
@@ -144,6 +145,13 @@ fn main() { | |
.value_name("FILE") | ||
.help("Export the timing results to the given file in csv format."), | ||
) | ||
.arg( | ||
Arg::with_name("export-json") | ||
.long("export-json") | ||
.takes_value(true) | ||
.value_name("FILE") | ||
.help("Export the timing results to the given file in JSON format."), | ||
) | ||
.help_message("Print this help message.") | ||
.version_message("Show version information.") | ||
.get_matches(); | ||
|
@@ -173,17 +181,11 @@ fn main() { | |
}, | ||
}; | ||
|
||
// Initial impl since we're only doing csv files, expand once we have multiple | ||
// export types. Simplest probably to do the same for each, but check for | ||
// None manager on later additions (JSON, Markdown, etc.) | ||
let export_manager = match matches.value_of("export-csv") { | ||
Some(filename) => { | ||
let mut export_manager = create_export_manager(); | ||
export_manager.add_exporter(&ResultExportType::Csv(filename.to_string())); | ||
Some(export_manager) | ||
} | ||
None => None, | ||
let export_targets = ExportTargetList { | ||
json_file: matches.value_of("export-json"), | ||
csv_file: matches.value_of("export-csv"), | ||
}; | ||
let export_manager = create_exporter(export_targets); | ||
|
||
// We default Windows to NoColor if full had been specified. | ||
if cfg!(windows) && options.output_style == OutputStyleOption::Full { | ||
|
@@ -210,3 +212,25 @@ fn main() { | |
Err(e) => error(e.description()), | ||
} | ||
} | ||
|
||
struct ExportTargetList<'a> { | ||
json_file: Option<&'a str>, | ||
csv_file: Option<&'a str>, | ||
} | ||
|
||
fn create_exporter(targets: ExportTargetList) -> Option<ExportManager> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have an idea of how to refactor this slightly, but I can do that in a separate PR. I will add you as a reviewer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this was next on my list, after I brought the markdown branch into PR scope There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #46 |
||
if targets.json_file.is_none() && targets.csv_file.is_none() { | ||
return None; | ||
} | ||
|
||
let mut export_manager = create_export_manager(); | ||
|
||
if let Some(filename) = targets.json_file { | ||
export_manager.add_exporter(ResultExportType::Json(filename.to_string())); | ||
} | ||
|
||
if let Some(filename) = targets.csv_file { | ||
export_manager.add_exporter(ResultExportType::Csv(filename.to_string())); | ||
} | ||
Some(export_manager) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe consider using this
enum
just to track the type of export and pass the file path as a separate argument? Just a thought.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, as I was looking at it, these two pieces of data are intrinsically linked to one-another. I didn't really see any benefit in breaking them apart, just to have to bring them back together or store the file name somewhere else. However, I just noticed that there is no reason to derive clone on this type and it can safely be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #46