Add --color cli switch #320

mjpieters · 2022-07-04T15:35:34Z

This adds a --color switch to the pueue command, controlling when output will
include ANSI styles (colors, bolding). The default, auto matches the current
behaviour: style when connected to a TTY.

This PR refactors the way styling is handled (consolidating color choices and
styling into a single location struct), and this struct then tracks if styling
can be output or not.

This would fix #318.

Checklist

I picked the correct source and target branch.
I included a new entry to the CHANGELOG.md.
I checked cargo clippy and cargo fmt. The CI will fail otherwise anyway.
(If applicable) I added tests for this feature or adjusted existing tests.
(If applicable) I checked if anything in the wiki needs to be changed.

mjpieters · 2022-07-04T16:19:06Z

I wonder why the build failed, but mostly why the target is aarch64-apple-ios. I'm working on a M1Pro, target aarch64-apple-darwin, perhaps that was intended?

mjpieters · 2022-07-04T16:26:26Z

Quick review assistance notes:

my (merged) PR Use style_text to style group header #319 fix to handle group header styling is already included in this PR.
I moved dark/light handling into the style_text method because the old approach of using .green() / .red() / ... methods is easily forgotten when writing new code. The style_text() method can trivially handle picking the right colours without developers of other areas of the code needing to remember.

mjpieters · 2022-07-04T16:48:40Z

I wonder why the build failed, but mostly why the target is aarch64-apple-ios. I'm working on a M1Pro, target aarch64-apple-darwin, perhaps that was intended?

Ah, no, it's more of a general issue with the rust toolchain, as the same issues appeared on the main branch.

Nukesor · 2022-07-05T16:35:16Z

Thanks for your contribution!

Yes, the CI seems to be broken for mac due to some changes in Rust v1.62.
I'll have a detailed look at your PR as soon as I find some time. I already took a first glance and the changes looked quite intuitive :)

mjpieters · 2022-07-06T11:39:18Z

Yes, the CI seems to be broken for mac due to some changes in Rust v1.62.

Could you elaborate? I created the PR using 1.62 on a Mac (Macbook Pro 16", M1 Pro CPU, on macos 12.14). I see no issues, but I didn't test every feature either.

I'll have a detailed look at your PR as soon as I find some time. I already took a first glance and the changes looked quite intuitive :)

I'll go over it once more as I see that some of the client/display/state code uses comfy-table directly, which means that my changes to how the dark-theme colours are used may be affected.

mjpieters · 2022-07-06T11:52:19Z

I'll go over it once more as I see that some of the client/display/state code uses comfy-table directly, which means that my changes to how the dark-theme colours are used may be affected.

I did indeed break dark styles for tables; I see two options to remedy this:

Revert the part of my refactor where the OutputStyle struct takes over swapping colours, putting the responsibility back on any code using colours.
Add a style_cell() method to the struct that lets you create styled comfy-table cells.

I'd pick the latter myself, so I'll just update the middle commit to do that. The other option would be to drop that commit.

mjpieters · 2022-07-06T12:51:17Z

So, the iOS build failure there was due to the cross-compilation toolchain not being installed, I fixed this in this PR by adding an extra commit that adds a target: line. :-)

I'll make that a separate PR against master, and I've removed the commit from this PR.

Now, the PR build with the commit fails on the clippy issues you already fixed on master :-P

codecov-commenter · 2022-07-06T17:44:03Z

Codecov Report

Merging #320 (adb59fb) into development (271b690) will decrease coverage by 0.13%.
The diff coverage is 1.18%.

@@               Coverage Diff               @@
##           development     #320      +/-   ##
===============================================
- Coverage        53.13%   52.99%   -0.14%     
===============================================
  Files               70       70              
  Lines             4705     4721      +16     
===============================================
+ Hits              2500     2502       +2     
- Misses            2205     2219      +14

Impacted Files	Coverage Δ
client/client.rs	`0.00% <0.00%> (ø)`
client/commands/format_state.rs	`0.00% <0.00%> (ø)`
client/commands/wait.rs	`0.00% <0.00%> (ø)`
client/display/group.rs	`0.00% <0.00%> (ø)`
client/display/helper.rs	`0.00% <0.00%> (ø)`
client/display/log/local.rs	`0.00% <0.00%> (ø)`
client/display/log/mod.rs	`0.00% <0.00%> (ø)`
client/display/log/remote.rs	`0.00% <0.00%> (ø)`
client/display/mod.rs	`0.00% <0.00%> (ø)`
client/display/state.rs	`0.00% <0.00%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 271b690...adb59fb. Read the comment docs.

mjpieters · 2022-07-06T17:51:55Z

Is the Windows test failure a flakey test? It also failed for a build of my toolchain PR but not when that PR was merged, but there the windows build was cancelled due to a codecov upload failure.

Nukesor · 2022-07-14T19:23:53Z

@mjpieters Yep, the windows test is a flaky one :D There's already an open ticket on this:

#315

I tried to increase the wait threshold several times, but this seems a different problem at hand. The original implementor of the windows code didn't came up with a fix either and just suggested to increase the wait threshold.

client/client.rs

client/display/style.rs

client/display/state.rs

Nukesor · 2022-07-16T12:22:15Z

Thanks again for your contributions! I'm always very happy when I get such high-quality contributions!

I'm also impressed by your detailed review and improvement of this project's CI! I definitely learned a few new things :)

Much appreciated!

Nukesor · 2022-07-16T13:18:46Z

FYI I decided to ignore the flaky windows test for now.

Previously it only occurred every now and then, but recently it showed up much more consistently. My initial assuption would be that this has something to do with the current load on the Github's windows CI machines.

This consolidates output style responsibility into a single struct. - Rename the Color struct to OutputStyle - Rename the module to 'style' - Move the 'style_text' helper into a method on OutputStyle - Update all uses to use `style` as the variable name.

- Remove the red/green/yellow/white methods, and instead have style_text swap out colours when styling. - Add a styled_cell method to ensure all Comfy-table output is following the same rules.

The default 'auto' enables styled output when connected to a tty. The other options are 'always' and 'never'.

mjpieters

This is your project, so I'll happily follow your style here.

And I agree that the Cell:new() API is cleaner. I didn't want to completely replicate that API (return a custom Cell struct which ignores chained fg() and add_attribute() calls when colours are disabled), perhaps this is something that the upstream comfy_table project should provide.

Nukesor · 2022-07-17T12:39:24Z

Review adjustments look good to me :)
I also tested the changes manually and everything worked just as expected.

Thanks again for your contribution!

I'll try to release a minor version in the near future :)

mjpieters force-pushed the cli-color-option branch 2 times, most recently from ac0ad03 to fbc5187 Compare July 4, 2022 16:09

mjpieters force-pushed the cli-color-option branch from fbc5187 to de6dfd4 Compare July 4, 2022 16:40

mjpieters force-pushed the cli-color-option branch from de6dfd4 to fbf8696 Compare July 6, 2022 12:36

mjpieters mentioned this pull request Jul 6, 2022

CI: make sure the target toolchain is available #321

Merged

5 tasks

mjpieters force-pushed the cli-color-option branch 2 times, most recently from fbf8696 to 578f690 Compare July 6, 2022 17:35

Nukesor requested changes Jul 14, 2022

View reviewed changes

client/client.rs Outdated Show resolved Hide resolved

client/display/style.rs Outdated Show resolved Hide resolved

client/display/state.rs Outdated Show resolved Hide resolved

mjpieters added 3 commits July 17, 2022 12:00

Refactor output styling

f97a239

This consolidates output style responsibility into a single struct. - Rename the Color struct to OutputStyle - Rename the module to 'style' - Move the 'style_text' helper into a method on OutputStyle - Update all uses to use `style` as the variable name.

Move light/dark color detail into OutputStyle

7bc6ac5

- Remove the red/green/yellow/white methods, and instead have style_text swap out colours when styling. - Add a styled_cell method to ensure all Comfy-table output is following the same rules.

Add --color command-line switch

adb59fb

The default 'auto' enables styled output when connected to a tty. The other options are 'always' and 'never'.

mjpieters commented Jul 17, 2022

View reviewed changes

mjpieters force-pushed the cli-color-option branch from 578f690 to adb59fb Compare July 17, 2022 11:15

Nukesor approved these changes Jul 17, 2022

View reviewed changes

Nukesor merged commit 910a333 into Nukesor:development Jul 17, 2022

Nukesor mentioned this pull request Jul 17, 2022

Add --color switch to control use of colours #318

Closed

mjpieters deleted the cli-color-option branch October 17, 2022 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --color cli switch #320

Add --color cli switch #320

mjpieters commented Jul 4, 2022

mjpieters commented Jul 4, 2022

mjpieters commented Jul 4, 2022

mjpieters commented Jul 4, 2022

Nukesor commented Jul 5, 2022

mjpieters commented Jul 6, 2022

mjpieters commented Jul 6, 2022

mjpieters commented Jul 6, 2022 •

edited

codecov-commenter commented Jul 6, 2022 •

edited

mjpieters commented Jul 6, 2022

Nukesor commented Jul 14, 2022 •

edited

Nukesor commented Jul 16, 2022 •

edited

Nukesor commented Jul 16, 2022

mjpieters left a comment

Nukesor commented Jul 17, 2022 •

edited

Add --color cli switch #320

Add --color cli switch #320

Conversation

mjpieters commented Jul 4, 2022

Checklist

mjpieters commented Jul 4, 2022

mjpieters commented Jul 4, 2022

mjpieters commented Jul 4, 2022

Nukesor commented Jul 5, 2022

mjpieters commented Jul 6, 2022

mjpieters commented Jul 6, 2022

mjpieters commented Jul 6, 2022 • edited

codecov-commenter commented Jul 6, 2022 • edited

Codecov Report

mjpieters commented Jul 6, 2022

Nukesor commented Jul 14, 2022 • edited

Nukesor commented Jul 16, 2022 • edited

Nukesor commented Jul 16, 2022

mjpieters left a comment

Choose a reason for hiding this comment

Nukesor commented Jul 17, 2022 • edited

mjpieters commented Jul 6, 2022 •

edited

codecov-commenter commented Jul 6, 2022 •

edited

Nukesor commented Jul 14, 2022 •

edited

Nukesor commented Jul 16, 2022 •

edited

Nukesor commented Jul 17, 2022 •

edited