benchmark_color: fix auto option #699
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
As prevously written, "--benchmark_color=auto" was treated as true,
because IsTruthyFlagValue("auto") returned true. The fix is to
rely on IsColorTerminal test only if the flag value is "auto",
and fall back to IsTruthyFlagValue otherwise. I also integrated
force_no_color check into the same block.
b2fcc36 to
f5009d7
Compare
|
CLAs look good, thanks! |
|
✅ Build benchmark 1509 completed (commit 5231f34aa8 by @iillyyaa) |
|
Looks reasonable, but is there any reasonable way to test this? |
|
Roman, I'm sure there is a way, but my cmake-fu is not up to snuff to come up with it. I'd appreciate if you accepted the fix as is. If tests must be provided, I will do my best, but can't make promises on the timing of the update. |
There was a problem hiding this comment.
The new code looks reasonable, but i'll leave this for @dominichamon.
|
i don't think cmake is required for tests, it would be a googletest unit test of the function, but you may have to use gmock to mock out IsColorTerminal... I'm ok with that waiting for another time. |
As prevously written, "--benchmark_color=auto" was treated as true,
because IsTruthyFlagValue("auto") returned true. The fix is to
rely on IsColorTerminal test only if the flag value is "auto",
and fall back to IsTruthyFlagValue otherwise. I also integrated
force_no_color check into the same block.
As prevously written, "--benchmark_color=auto" was treated as true,
because IsTruthyFlagValue("auto") returned true. The fix is to
rely on IsColorTerminal test only if the flag value is "auto",
and fall back to IsTruthyFlagValue otherwise. I also integrated
force_no_color check into the same block.
(Aside for @dominichamon: I noticed that you mentioned in the ticket that a PR was in progress, but I couldn't locate it.)
Fixes #559.