Skip to content
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

Merged
merged 1 commit into from
Mar 20, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ cfg-if = "0.1.2"
csv = "1.0.0-beta.5"
serde = "1.0.33"
serde_derive = "1.0.33"
serde_json = "1.0.11"

[target.'cfg(not(windows))'.dependencies]
libc = "0.2"
Expand Down
22 changes: 11 additions & 11 deletions src/hyperfine/export/csv.rs
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 {}
}
}
29 changes: 29 additions & 0 deletions src/hyperfine/export/json.rs
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 {}
}
}
39 changes: 29 additions & 10 deletions src/hyperfine/export/mod.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
mod csv;
mod json;

use self::csv::CsvExporter;
use self::json::JsonExporter;

use std::io::Result;
use std::io::{Result, Write};
use std::fs::File;

use hyperfine::internal::Second;

Expand Down Expand Up @@ -51,16 +54,19 @@ impl ExportEntry {

/// The ResultExportType enum is used to denote the desired form
/// of exporter to use for a given file.
#[derive(Clone)]
pub enum ResultExportType {
Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #46

/// Export to a csv file with the provided name
Csv(String),
/// Export to a JSON file
Json(String),
}

/// A ResultExporter is responsible for writing all results to the
/// appropriate file
trait ResultExporter {
/// Write all entries to the target destination
fn write(&self, values: &Vec<ExportEntry>) -> Result<()>;
fn write(&self, values: &Vec<ExportEntry>) -> Result<Vec<u8>>;
}

/// Create a new ExportManager
Expand All @@ -70,23 +76,36 @@ pub fn create_export_manager() -> ExportManager {
}
}

/// The Exporter is the internal implementation of the ExportManager
/// The ExportManager handles the management of multiple file
/// exporters.
pub struct ExportManager {
exporters: Vec<Box<ResultExporter>>,
exporters: Vec<ResultExportType>,
}

impl ExportManager {
pub fn add_exporter(&mut self, for_type: &ResultExportType) {
match for_type {
&ResultExportType::Csv(ref file_name) => self.exporters
.push(Box::from(CsvExporter::new(file_name.clone()))),
};
/// Add an additional exporter to the ExportManager
pub fn add_exporter(&mut self, for_type: ResultExportType) {
self.exporters.push(for_type);
}

/// Write the given results to all Exporters contained within this manager
pub fn write_results(&self, to_write: Vec<ExportEntry>) -> Result<()> {
for exp in &self.exporters {
exp.write(&to_write)?;
let (exporter, filename): (Box<ResultExporter>, &str) = match exp {
&ResultExportType::Csv(ref file) => (Box::from(CsvExporter::new()), file),
&ResultExportType::Json(ref file) => (Box::from(JsonExporter::new()), file),
};

let file_content = exporter.write(&to_write)?;
write_to_file(filename, file_content)?;
}
Ok(())
}
}

/// Write the given content to a file with the specified name
fn write_to_file(filename: &str, content: Vec<u8>) -> Result<()> {
let mut file = File::create(filename)?;
file.write_all(&content)?;
Ok(())
}
46 changes: 35 additions & 11 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ extern crate serde;

#[macro_use]
extern crate serde_derive;
extern crate serde_json;
extern crate statistical;

cfg_if! {
Expand All @@ -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) -> ! {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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 {
Expand All @@ -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> {
Copy link
Owner

@sharkdp sharkdp Mar 20, 2018

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Owner

Choose a reason for hiding this comment

The 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)
}