search

BurntSushi / cargo-benchcmp

A small utility to compare Rust micro-benchmarks.
The Unlicense
343 stars 21 forks source link

Coloured regressions/improvements #7

Closed Apanatshka closed 8 years ago

Apanatshka commented 8 years ago

As a side-effect of switching from tabwriter to prettytable-rs, we can now do coloured terminal output. It's a small change to get red lines for regressions and green lines for improvements. It almost makes the --regressions/--improvements flags superfluous :)

I can implement this feature quite easily (I did it before but I can't find the code just now), I may be able to squeeze it into a break or do it right after work. My modem is broken so I'm currently relying on internet access at work 😐

BurntSushi commented 8 years ago

OMG yes! This would be a huge improvement and I didn't even realize prettytable-rs had that.

One thing with colors though is that you typically have to be a little clever about whether you actually emit the escape sequences. For example, I know a lot of programs that do color actually detect whether you're piping output to a file or not, in which case, coloring would be disabled. In this case, while benchcmp is probably most often used in the terminal, it's totally plausible to want to save the results in a file for future reference.

Does prettytable-rs support that?

I can see that cargo is smart enough about this, so my guess is that there's something in the ecosystem that knows how to do it. (I'm at work and don't have time to carefully look into this myself at the moment.)

BurntSushi commented 8 years ago

With that said, I think I'd be totally fine landing a change that always does colors with a new flag that lets one disable them, so long as there's a clear path to making this smarter.

Apanatshka commented 8 years ago

Yes, prettytable-rs allows for auto-detection. You can also force it into either of the two modes. So we can do the same as cargo does: have a flag --color where the default setting is auto but you can also set it to always or never.

One question: can we also have a flag alias --colour? I know I will run into that one once in a while, and there is precedent

BurntSushi commented 8 years ago

Doing what Cargo does is a good idea.

I'd rather have no aliases, especially for a flag that will probably be rarely used in the presence of auto detection, and because the failure mode is obvious and fast.

Apanatshka commented 8 years ago

I started a PR, so I think we can close this.

BurntSushi commented 8 years ago

Usually issues stay open until the PR is merged. :-)

Apanatshka commented 8 years ago

Oh, sorry 😅
Maybe some steps of the contribution process should be documented in a CONTRIBUTING.md?