ansible-builder icon indicating copy to clipboard operation
ansible-builder copied to clipboard

Allow disabling text colors

Open Shrews opened this issue 10 months ago • 3 comments

Allows control of when builder output text is colorized.

Honors the NO_COLOR and FORCE_COLOR environment variables.

Fixes #716

Shrews avatar Feb 19 '25 21:02 Shrews

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...

Shrews avatar Mar 25 '25 13:03 Shrews

@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): https://github.com/Textualize/rich/issues/2924#issuecomment-2757673602 / https://github.com/Textualize/rich/pull/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.

webknjaz avatar Apr 23 '25 19:04 webknjaz

@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".

I decided that simply supporting NO_COLOR is the simplest thing to do, and makes it much less confusing as to how it should operate.

Shrews avatar Aug 05 '25 15:08 Shrews

NOTE: The tox-dev framework we use in CI sets FORCE_COLOR=1. If we were to support that option, it would need to be at a lower priority than NO_COLOR, because otherwise our tests (as they currently exist) would break with Python>=3.14. Although there have been discussions about whether or not NO_COLOR should trump the other option, I do not see a consensus, or anything written in stone about that, except that, as of this writing, the code example on force-color.org still suggests that FORCE_COLOR is top priority. Leaving support for that option out of this change, at least for now, seems sensible.

Shrews avatar Aug 05 '25 19:08 Shrews