Add export manager and CSV export capability #41

stevepentland · 2018-03-17T23:04:58Z

The work done here is toward #38 and includes an exporter for CSV and a manager to handle multiple exporters.

It is done to allow multiple exporters (csv, json, etc.) to be used at one time with the only requirement being to add a new enum value to export types and the exporter itself that is responsible for writing. JSON should be a very easy output type to add at this point, and I will move to that next once this PR is closed.

sharkdp

Thank you very much!! This looks great.

I have added a few review comments. Curious to hear what you think.

sharkdp · 2018-03-18T18:14:16Z

Cargo.toml

@@ -15,6 +15,9 @@ indicatif = "0.9"
 statistical = "0.1"
 atty = "0.2.2"
 cfg-if = "0.1.2"
+csv = "1.0.0-beta.5"


Do we really need to rely on a beta version here? Would the latest stable version work, too?

I'll check, our use case is pretty simple so I'm hopeful older versions would work

It appears that serde support is not available in pre 1.0.0 versions. I tried 0.15.0 but it is requiring some rather significant changes to serialization, writing, and so on. Not to mention that I can no longer find all the documentation for that version (Encodable trait results in 404). It may be reasonable to use the beta for now, which will also allow keeping serde support betwen json and csv

Okay, thanks for checking!

sharkdp · 2018-03-18T18:19:17Z

src/hyperfine/benchmark.rs

@@ -136,6 +137,7 @@ pub fn run_benchmark(
    cmd: &str,
    shell_spawning_time: TimingResult,
    options: &HyperfineOptions,
+    exporter: &mut Option<Box<ExportManager>>,


It would be great if we could change the API a little bit. Instead of run_benchmark taking a mutable ExportManager and calling add_result on it, couldn't the function simply return the results (The ExportEntry)?

sharkdp · 2018-03-18T18:21:42Z

src/hyperfine/export/csv.rs

+}
+
+impl CsvExporter {
+    pub fn new(file_name: String) -> Self {


Could the file-handling part be generalized and moved to export/mod.rs? Otherwsise, every exporter type would have to handle this on its own. Instead, each exporter could probably just return a String with the file contents.

It could, I wanted to let particular serializer libraries handle output to files in their own way. I can look into refactoring this later to unify file handling however

sharkdp · 2018-03-18T18:22:31Z

src/hyperfine/export/mod.rs

+    /// The command that was run
+    command: String,
+    /// The mean run time
+    mean: f64,


Can we use Second instead of f64 here? Does this work with Serialize?

sharkdp · 2018-03-18T18:24:31Z

src/hyperfine/export/mod.rs

+
+/// The ResultExportType enum is used to denote the desired form
+/// of exporter to use for a given file.
+pub enum ResultExportType {


Do we need this enum? ResultExporter is a trait, so we should be able to pass the trait objects directly.

I wanted to take away all reasoning about what would be added as an exporter and leave it up to the manager to create the appropriate type. The internal operation of exporting isn't really important to external calls, only that they want a json, csv, or other file format. It makes a single entry point for all exports instead of making a new exporter and passing it in

Ok, sounds reasonable.

sharkdp · 2018-03-18T18:25:12Z

src/hyperfine/export/mod.rs

+}
+
+/// The ExportManager handles coordination of multiple ResultExporters
+pub trait ExportManager {


Do we need ExportManager to be a trait? Couldn't this just be a struct?

Sure, I was just trying to extract away all implementation, but I can switch that one out

I'd prefer a simple struct. I don't think we need ExportManager as an abstraction/interface here. There is just one of them. I think it would also simplify the code and the readability.

sharkdp · 2018-03-18T18:26:33Z

src/main.rs

+            Arg::with_name("export-csv")
+                .long("export-csv")
+                .takes_value(true)
+                .multiple(true)


Do we need to allow for multiple options here? Wouldn't all these files have the same contents?

They would yes, however as there is no down side to writing to multiple files, I figured offering the option to write to more than one is pretty easy. I don't generally want to try and guess user's desires since we can easily write 1 or more files, in multiple formats.

Fair point. However, I don't think that this will be an actual use case. Even if it would, the user could simply copy the file afterwards. Also, I think there is a downside to defining it multiple = true: the generated help text will look like --export-csv <file>... (or similar) which might confuse some users ("do I have to specify multiple files? one for each benchmark command?").

Ok, I'll remove multiples from this one, and from the json branch (rebasing back on these changes anyway)

sharkdp · 2018-03-18T18:27:52Z

src/main.rs

+fn run(
+    commands: &Vec<&str>,
+    options: &HyperfineOptions,
+    exporter: &mut Option<Box<ExportManager>>,


Same as above: I would rather like run to return the results instead of passing a mutable ExportManager.

Sure, that'll simplify everything

Manager is now a simple struct, that no longer stores the results, but instead takes in a Vec<ExportEntry> and has all exporters write from this collection. Benchmark and run both return their results, no more mut manager. Change f64 in ExportEntry to hyperfine::internal::Second. Remove multiple export files for a single type.

stevepentland · 2018-03-18T19:50:26Z

The only things left out from review:

file handling: still a per-exporter basis, as they simply create the file according to how the external lib prefers to write output. This can be changed easily once an additional exporter (json) is present
(Done in JSON branch)
csv lib still beta: Seems the best option at this point, once burntsushi puts out a final 1.0.0 release we can switch to that.

sharkdp · 2018-03-19T06:53:26Z

Fantastic - thank you!

stevepentland added 3 commits March 17, 2018 18:59

Add export manager and CSV export capability

3311a3f

Make it possible to export to multiple files at once

243db09

Do not copy export items between exporters, write from central vec

717fa46

This was referenced Mar 18, 2018

Export to JSON #42

Closed

Json export #43

Closed

sharkdp self-requested a review March 18, 2018 18:13

sharkdp requested changes Mar 18, 2018

View reviewed changes

Add min, max to output. They were missed

919a46f

sharkdp approved these changes Mar 19, 2018

View reviewed changes

sharkdp merged commit ce79d81 into sharkdp:master Mar 19, 2018

This was referenced Mar 19, 2018

Add export options #38

Closed

Export to Markdown #44

Closed

stevepentland deleted the results-export branch March 19, 2018 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add export manager and CSV export capability #41

Add export manager and CSV export capability #41

stevepentland commented Mar 17, 2018

sharkdp left a comment

sharkdp Mar 18, 2018

stevepentland Mar 18, 2018

stevepentland Mar 18, 2018

sharkdp Mar 18, 2018

sharkdp Mar 18, 2018

sharkdp Mar 18, 2018

stevepentland Mar 18, 2018

sharkdp Mar 18, 2018

stevepentland Mar 18, 2018

sharkdp Mar 18, 2018

stevepentland Mar 18, 2018 •

edited

sharkdp Mar 18, 2018

sharkdp Mar 18, 2018

stevepentland Mar 18, 2018

sharkdp Mar 18, 2018

sharkdp Mar 18, 2018

stevepentland Mar 18, 2018

sharkdp Mar 18, 2018

stevepentland Mar 18, 2018

sharkdp Mar 18, 2018

stevepentland Mar 18, 2018

stevepentland commented Mar 18, 2018 •

edited

sharkdp commented Mar 19, 2018

Add export manager and CSV export capability #41

Add export manager and CSV export capability #41

Conversation

stevepentland commented Mar 17, 2018

sharkdp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevepentland Mar 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevepentland commented Mar 18, 2018 • edited

sharkdp commented Mar 19, 2018

stevepentland Mar 18, 2018 •

edited

stevepentland commented Mar 18, 2018 •

edited