Skip to content
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

Add requirement for TERM not to be dumb to enable colorization #1287

Merged
merged 3 commits into from
Feb 10, 2025

Conversation

snosov1
Copy link
Contributor

@snosov1 snosov1 commented Jan 31, 2025

Hello!

This is a trivial change to disable coloring in case the terminal is dumb. Personally, I encountered the problem in Emacs terminal/compilation buffers - the coloring symbols are just printed as is (i.e. ASCII).

Technically, some more comprehensive logic can be implemented to derive color support from the TERM variable - but I'm not sure it's actually warranted. toe /usr/share/terminfo on my Ubuntu instance returns about 40 different terminal types - for some, it's clear whether the color is supported or not, for others - not that clear.

Anyway, just opting out in case of dumb terminal seems like a sensible thing to do (and it solves my problem in Emacs).

Please, let me know what you think.

@Delgan
Copy link
Owner

Delgan commented Feb 10, 2025

Hey @snosov1, thanks for your contribution! 👍

I agree with you that it seems fair to disable colors in this case. This is also what is done by supports-color.js which I sometimes use as a reference, due to the completeness of their checks.

I'm merging the PR as-is but note that I'll probably move the check to the code block right above. This is because I think the special case for $TERM="dumb" should only apply when stream is the default standard output/error. If stream happens to be a custom IOBase provided by the user, the $TERM value should become irrelevant.

@Delgan Delgan merged commit 2f7007a into Delgan:master Feb 10, 2025
20 checks passed
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.

2 participants