Skip to content

Allow disabling text colors #727

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: devel
Choose a base branch
from
Open

Allow disabling text colors #727

wants to merge 4 commits into from

Conversation

Shrews
Copy link
Contributor

@Shrews Shrews commented Feb 19, 2025

Allows control of when builder output text is colorized.

Honors the NO_COLOR and FORCE_COLOR environment variables.

Fixes #716

@Shrews Shrews requested a review from a team as a code owner February 19, 2025 21:28
@Shrews Shrews marked this pull request as draft February 19, 2025 21:46
@github-actions github-actions bot added the docs Changes to documentation label Feb 20, 2025
@Shrews Shrews marked this pull request as ready for review February 20, 2025 20:37
Copy link
Member

@nitzmahone nitzmahone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks okay, though I'd question if it warrants a dedicated CLI arg- for most things we guard that kind of addition pretty carefully.

I'm also curious how hard it is to get the build output to include color codes- my worry is that having this option could set an expectation that we're going to also filter or otherwise force-disable generation of color codes from the build, which could be problematic.

@Shrews
Copy link
Contributor Author

Shrews commented Mar 25, 2025

Overall looks okay, though I'd question if it warrants a dedicated CLI arg- for most things we guard that kind of addition pretty carefully.

Ack. I can remove that bit.

I'm also curious how hard it is to get the build output to include color codes- my worry is that having this option could set an expectation that we're going to also filter or otherwise force-disable generation of color codes from the build, which could be problematic.

Not sure I catch what you mean here...

@github-actions github-actions bot removed the docs Changes to documentation label Mar 26, 2025
Copy link

@webknjaz
Copy link
Member

@Shrews there's a corner case in Rich interpreting FORCE_COLOR as "also produce ANSI-sequences for bold" with NO_COLOR as "only disable color-related ANSI but not bold".

This resulted in us settling on an additional variable TTY_COMPATIBLE that only supports 1 or 0 explicit values (auto-detect otherwise): Textualize/rich#2924 (comment) / Textualize/rich#3675.

I'm wondering if this PR would benefit from also consulting that var.

Also, here's how termcolor handles these vars (it doesn't yet look into TTY_COMPATIBLE tho): https://github.com/termcolor/termcolor/blob/fd08149/src/termcolor/termcolor.py#L92-L125. In general, I think it'd be beneficial closely following whatever logic widely-used libs implement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ansible-builder does not have option to disable console ANSI coloring
3 participants