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

Enable FORCE_COLOR and NO_COLOR for terminal colouring #10260

Merged
merged 3 commits into from Mar 16, 2022

Conversation

hugovk
Copy link
Contributor

@hugovk hugovk commented Mar 12, 2022

Subject: Allow these widely-used environment variables to improve readability of output

Feature or Bugfix

  • Feature

Purpose

Colour can help diagnose problems, and a common place for Sphinx and the need to diagnose problems is on a CI.

However, because GitHub Actions doesn't use a tty, output of tools which autodetect the terminal output is usually monochrome.

Normally you only want to enable colour on a tty, because if you're piping data between commands (not a tty) the colour codes can mess up parsing of piped output. But that's not the case here.

That's why many tools and CI setups use the FORCE_COLOR environment variable, and/or command-line options to turn it back on.

Other tools for reference:

Detail

  • Like other tools, NO_COLOR takes precedence
  • Keep the Windows-specific check before because it initialises colorama
  • FORCE_COLOR next

Relates

@tk0miya tk0miya added this to the 4.5.0 milestone Mar 13, 2022
Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

LGTM. Could you add mentions for these envvars to doc/man/sphinx-build.rst too?

@hugovk
Copy link
Contributor Author

hugovk commented Mar 13, 2022

Copy link
Contributor

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

After digging for few good number of minutes about why sphinx does not use colors i reached this one.

Yes, current cli arguments are not automation/scripting friendly and we are much better reusing existing standard variables for controlling the colors.

@ssbarnea
Copy link
Contributor

@tk0miya Can you please have another look, it seems fixed. I am really eager to get this fix in especially as the older tox trick with {tty:--color} no longer works, for the same reasons. TTY is not the same as supporting color or not.

ssbarnea added a commit to ssbarnea/ansible-language-server that referenced this pull request Mar 16, 2022
ssbarnea added a commit to ansible/ansible-language-server that referenced this pull request Mar 16, 2022
@tk0miya
Copy link
Member

tk0miya commented Mar 16, 2022

Thank you for your update. Merging now!

@tk0miya tk0miya merged commit 4e9c101 into sphinx-doc:4.x Mar 16, 2022
@hugovk hugovk deleted the colour-env-vars branch March 16, 2022 17:34
The-Compiler added a commit to qutebrowser/qutebrowser that referenced this pull request Mar 30, 2022
The-Compiler added a commit to qutebrowser/qutebrowser that referenced this pull request Mar 30, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants