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 support for NO_COLOR environment variable #321

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EyLuismi
Copy link

As issue #143 describes, NO_COLOR is becoming an established environment variable to turn off colour output in environments that may not accept them, such as Log aggregators. More info at: https://no-color.org/

This PR aims to support it by adding an Environment Variable Manager that can be passed down to the Colorizer format and helps with test mocking.

@DABH
Copy link
Contributor

DABH commented Oct 1, 2024

Hey there, thanks for this idea! In principle I have no objection. It seems like this might be a bit over-engineered though - why do we need the env manager? I.e. why not just

this.options.disableColor = process.env.NO_COLOR &&
        process.env.NO_COLOR !== '0' &&
        process.env.NO_COLOR !== 'false';

? I think that's the style we generally use across the winston stack for things (no separate options structs etc.) so that might be a bit more stylistically consistent here (and fewer lines of code). Let me know your thoughts though. Thanks again for your contribution!

@EyLuismi
Copy link
Author

EyLuismi commented Oct 1, 2024

Hi! Yes, that was my first approach, but then, the test failed depending on your NO_COLOR ENV var. And using process.env there and in the tests maybe creating unknown side effects seemed like something harder to keep track in the future for the project. I could re-create the PR using that approach if you think is better. Thanks for your time!

@DABH
Copy link
Contributor

DABH commented Nov 11, 2024

Hmm maybe @wbt can take a look and opine sometime on this one

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