Add new style value to options, include in args #24

stevepentland · 2018-01-22T20:55:31Z

This is a preliminary pull request for consultation on the current direction of the changes. The new option is present in both the args and HyperfineOptions but is not yet used.

sharkdp

Looks good so far!

You can just keep pushing to this branch.

sharkdp · 2018-01-23T06:59:27Z

.gitignore

@@ -1,3 +1,3 @@
-
+.vscode/


It feels like this would be something for a global .gitignore file. https://help.github.com/articles/ignoring-files/#create-a-global-gitignore

Sure, no problem

sharkdp · 2018-01-23T07:01:36Z

src/main.rs

+                .long("style")
+                .short("s")
+                .takes_value(true)
+                .value_name("OPT")


What does OPT stand for? Maybe change this to TYPE?

It was chosen to represent "Option" while staying in line with your existing 3 letter notation for placeholders in the help output (NUM, CMD, etc.) as I didn't want to stray from a current convention.

That's okay. 3 letters was just a coincidence

sharkdp · 2018-01-23T07:03:13Z

src/main.rs

+                .value_name("OPT")
+                .possible_values(&["auto", "basic", "full"])
+                .help(
+                    "Set output style type. If set to 'basic', all colors and special \


We could maybe add ( default: auto).

It affected the output when hyperfine was called with no arguments. When I get home from work today I'll update this comment.

Actually I misunderstood what you meant by default. I was talking about when I added it as a call on Arg (default_value("auto")). But yes, I can add (default: auto) to the help output.

sharkdp · 2018-01-24T19:31:09Z

Just a heads up: I switched from ansi_term to the colored crate.

stevepentland · 2018-01-24T21:46:38Z

Thanks for the heads up, I've incorporated the changes from your upstream. I'm still working on devising a method to satisfy the new output options without having conditional checks everywhere output is done so I wasn't too affected.

However, the 'basic' output is rather noisy, and a better output style needs to be determined.

stevepentland · 2018-01-24T22:49:06Z

@sharkdp I used an indirect approach to the output, so no individual consumer would have to worry about the style of output. I have only used it in one location and still need to do cleanup, but wanted to get your thoughts. This method only leaves the couple of lone println statements to have to worry about colors in this case.

sharkdp · 2018-01-25T06:57:34Z

@stevepentland I haven't completely reviewed the code (yet) but that looks like a cool idea.

Another option we could explore would be to:

Simply hide the progress bars: completely https://docs.rs/indicatif/0.8.0/indicatif/struct.ProgressBar.html#method.hidden
Use a different style without spinners, etc.: https://docs.rs/indicatif/0.8.0/indicatif/struct.ProgressBar.html#method.set_style

Actually, it looks like the indicatif progress bars do not show up in log files if you redirect stdout:

hyperfine 'sleep 0.1' > test.log
# or:
hyperfine 'sleep 0.1' | tee test.log

stevepentland · 2018-01-25T22:31:54Z

@sharkdp Thanks for the ideas. I played around with them a bit and found that the results are a mixed bag. I'll address them in order of your suggestion:

This option does work, but due to the progress bar still controlling the final output, the options for showing what is going on is lacking as everything is swallowed up. So reporting progress such as % complete on a line-by-line basis is no longer available.
Even without spinner we still have the situation where all TUI features are not disabled, such as overwriting the last line, so we're still at the mercy of what indicatif wants to do.

It seems, after some quick experimentation, that the most reliable way to ensure that all special formatting is eliminated during basic runs is to completely omit the bar as an output target.

Additional changes to make sure that all text is plain white when using basic mode.

stevepentland · 2018-01-27T03:40:09Z

@sharkdp I've changed back to checking output style both at progress bar and during println statements. While there is no progress indication in basic mode, the results are still presented in a legible way. I played around with it a bit and it seems pretty good.

sharkdp · 2018-01-27T08:29:02Z

src/hyperfine/benchmark.rs

-        cmd
-    );
+    let (benchark_text, current_num) = match options.output_style {
+        OutputStyleOption::Basic => ("Benchmark #".white(), (num + 1).to_string().white()),


Unfortunately, white() will also output ANSI sequences (�[37mBenchmark #�[0m�[37m1�[0m: sleep 0.1). Could we simply keep the colored version here and disable coloring globally via colored::control::set_override

dang, I'm embarrassed I hadn't seen that option when I was looking at the docs for the crate. Thanks for pointing it out. I'll make the change back to the original for println statements and set the global output

sharkdp · 2018-01-27T08:30:29Z

src/hyperfine/benchmark.rs

@@ -244,23 +252,49 @@ pub fn run_benchmark(
    let (user_str, user_unit) = format_duration_unit(user_mean, None);
    let system_str = format_duration(system_mean, Some(user_unit));

+    let (mean_start, delta, mean_val, stddev_val, user_val, sys_val) = match options.output_style {


The same here. I think we could just keep the old version and disable coloring globally.

Original attempt tried to directly set what would be output, this was more complex and did not actually solve the problem. Now the global setting of the colored crate is used to disable all colorization of output as soon as basic is noted.

stevepentland · 2018-01-27T16:36:38Z

@sharkdp I have made the changes you pointed out. Thanks again for directing me to that setting in the docs, I had completely missed it.

sharkdp · 2018-01-28T13:34:55Z

Thank you very much!

sharkdp · 2018-01-28T18:01:17Z

Released as v0.4.0

Add new style value to options, include in args

6b26ea1

sharkdp requested changes Jan 23, 2018

View reviewed changes

stevepentland added 2 commits January 24, 2018 16:36

Fix merge conflict due to new packages

20b53b3

Changes from code review

409bdb0

Initial change, progress bar works when used.

be2951a

However, the 'basic' output is rather noisy, and a better output style needs to be determined.

Remove output target and use progress bar with different styles.

6e20860

Additional changes to make sure that all text is plain white when using basic mode.

Had clap color setting reversed, fixed.

2740399

sharkdp requested changes Jan 27, 2018

View reviewed changes

Fixed coloring of output via global setting.

b1bb3f0

Original attempt tried to directly set what would be output, this was more complex and did not actually solve the problem. Now the global setting of the colored crate is used to disable all colorization of output as soon as basic is noted.

sharkdp approved these changes Jan 28, 2018

View reviewed changes

sharkdp merged commit 2e24ff0 into sharkdp:master Jan 28, 2018

sharkdp mentioned this pull request Jan 28, 2018

Add --color=auto/never/always option #20

Closed

stevepentland deleted the styleopt branch February 2, 2018 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new style value to options, include in args #24

Add new style value to options, include in args #24

stevepentland commented Jan 22, 2018

sharkdp left a comment

sharkdp Jan 23, 2018

stevepentland Jan 23, 2018

sharkdp Jan 23, 2018

stevepentland Jan 23, 2018 •

edited

Loading

sharkdp Jan 23, 2018

sharkdp Jan 23, 2018

stevepentland Jan 23, 2018

stevepentland Jan 24, 2018

sharkdp commented Jan 24, 2018

stevepentland commented Jan 24, 2018

stevepentland commented Jan 24, 2018

sharkdp commented Jan 25, 2018

stevepentland commented Jan 25, 2018

stevepentland commented Jan 27, 2018 •

edited

Loading

sharkdp Jan 27, 2018

stevepentland Jan 27, 2018

sharkdp Jan 27, 2018

stevepentland commented Jan 27, 2018

sharkdp commented Jan 28, 2018

sharkdp commented Jan 28, 2018

Add new style value to options, include in args #24

Add new style value to options, include in args #24

Conversation

stevepentland commented Jan 22, 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

stevepentland Jan 23, 2018 • edited Loading

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

sharkdp commented Jan 24, 2018

stevepentland commented Jan 24, 2018

stevepentland commented Jan 24, 2018

sharkdp commented Jan 25, 2018

stevepentland commented Jan 25, 2018

stevepentland commented Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevepentland commented Jan 27, 2018

sharkdp commented Jan 28, 2018

sharkdp commented Jan 28, 2018

stevepentland Jan 23, 2018 •

edited

Loading

stevepentland commented Jan 27, 2018 •

edited

Loading