Option to disable the default output entirely [#193] #224
Conversation
This commit allows to remove all non-errornous output of the tool.
src/hyperfine/benchmark.rs
Outdated
| @@ -221,17 +231,21 @@ pub fn run_benchmark( | |||
| options.failure_action, | |||
| None, | |||
| )?; | |||
| progress_bar.inc(1); | |||
| if let Some(pbar) = progress_bar.clone() { pbar.inc(1); } | |||
There was a problem hiding this comment.
We should avoid .clone()ing. You can probably use if let Some(ref pbar) = …
There was a problem hiding this comment.
Yeah, thanks, you were right, that works well and improves performance, although that (performance) doesn't seem crucial for this case?
There was a problem hiding this comment.
no, it's not. but I think its much cleaner without cloning it everywhere.
There was a problem hiding this comment.
yes, totally agree on that
There was a problem hiding this comment.
Coming to think of it, there is an even cleaner solution to write this, that we should probably follow. You can use map to "directly" call a function on the object inside the Option, allowing us to get rid of all the if statements. Should be something like
progress_bar.map(|bar| bar.inc(1));or
progress_bar.as_ref().map(|bar| bar.inc(1));There was a problem hiding this comment.
Hmm, that looks quite interesting, but do you think it's that readable?
There was a problem hiding this comment.
Yes, I think that's readable. Using .map(…) on Option, Result, or any kind of Iterator is very common in Rust code.
There was a problem hiding this comment.
Actually, seems quite nice when I researched a bit more. Updated the PR with that way
src/main.rs
Outdated
| @@ -54,8 +54,9 @@ fn main() { | |||
|
|
|||
| match res { | |||
| Ok(timing_results) => { | |||
| write_benchmark_comparison(&timing_results); | |||
| let ans = export_manager.write_results(timing_results, options.unwrap().time_unit); | |||
| let unwrapped = options.unwrap(); | |||
There was a problem hiding this comment.
instead of calling it unwrapped, we could call it options and shadow the original name.
The fact that we call unwrap here anyway is not really great, but that should be fixed elsewhere.
There was a problem hiding this comment.
updated with shodowing for now. do you have any ideas on re-writing in the way not to have to unwrap the options object?
|
Thanks for the review! Sorry for taking your time for those quite simple things, not too much in Rust yet and decided to improve myself in it during Hacktoberfest. Will update up to your comments today/tomorrow. |
|
@sharkdp , I have updated the PR as you have described. Again, thanks a lot for lots of useful comments. |
This PR addresses the #193 Feature request. It allows to pass
noneas a value for--styleoption.If it is used, then no output (unless they are errors).
@sharkdp , may be there is some sense to print warnings as well. What do you think?