Skip to content

Downgrade termcolor to prevent atty check which disables colors#86

Merged
NeffIsBack merged 2 commits intodevelopfrom
neff-fix-color
Oct 26, 2023
Merged

Downgrade termcolor to prevent atty check which disables colors#86
NeffIsBack merged 2 commits intodevelopfrom
neff-fix-color

Conversation

@NeffIsBack
Copy link
Member

See #82

@NeffIsBack NeffIsBack added the bug-fix This Pull Request fixes a bug label Oct 21, 2023
@Marshall-Hallenbeck
Copy link
Collaborator

@NeffIsBack why don't we just use #82 instead of downgrading...?

@NeffIsBack
Copy link
Member Author

NeffIsBack commented Oct 21, 2023

I am afraid that we enforce colors in terminals which aren't made for it leading to having the color code inside the output. There are other checks inside termcolor that gets overwritten by enforcing the color like environment variables that are supposed to disable all colors

@Marshall-Hallenbeck
Copy link
Collaborator

Do you have examples of what you mean? I think I understand but I'm not sure where it would be come a problem.

@NeffIsBack
Copy link
Member Author

image

Termcolor now checks if stdout isatty in 2 ways (line 124&125) where the second check fails in our case. You can see that you can enforce the color by either an ENV variable or the argument. This disables Environment variable checks where terminals could prevent colors from being displayed (line 117-120). I am not sure where this is used but i can imagine that there are terminal which doesn't interprete colors and therefore print out the raw color tags

@NeffIsBack
Copy link
Member Author

@Marshall-Hallenbeck merging this PR so we have a temporary fix until @XiaoliChan's PR is hopefully merged

termcolor/termcolor#56

@NeffIsBack NeffIsBack merged commit b21a1eb into develop Oct 26, 2023
@NeffIsBack NeffIsBack deleted the neff-fix-color branch October 26, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug-fix This Pull Request fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants