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

Set coloured output with a context manager #12134

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 3 additions & 8 deletions sphinx/cmd/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
from sphinx.util._io import TeeStripANSI
from sphinx.util.console import ( # type: ignore[attr-defined]
color_terminal,
nocolor,
colorize_output,
red,
terminal_safe,
)
Expand Down Expand Up @@ -249,11 +249,6 @@ def _validate_filenames(
parser.error(__('cannot combine -a option and filenames'))


def _validate_colour_support(colour: str) -> None:
if colour == 'no' or (colour == 'auto' and not color_terminal()):
nocolor()


def _parse_logging(
parser: argparse.ArgumentParser,
quiet: bool,
Expand Down Expand Up @@ -324,7 +319,7 @@ def build_main(argv: Sequence[str]) -> int:
args.confdir = _parse_confdir(args.noconfig, args.confdir, args.sourcedir)
args.doctreedir = _parse_doctreedir(args.doctreedir, args.outputdir)
_validate_filenames(parser, args.force_all, args.filenames)
_validate_colour_support(args.color)
color = not (args.color == 'no' or (args.color == 'auto' and not color_terminal()))
args.status, args.warning, args.error, warnfp = _parse_logging(
parser, args.quiet, args.really_quiet, args.warnfile)
args.confoverrides = _parse_confoverrides(
Expand All @@ -333,7 +328,7 @@ def build_main(argv: Sequence[str]) -> int:
app = None
try:
confdir = args.confdir or args.sourcedir
with patch_docutils(confdir), docutils_namespace():
with colorize_output(color), patch_docutils(confdir), docutils_namespace():
app = Sphinx(args.sourcedir, args.confdir, args.outputdir,
args.doctreedir, args.builder, args.confoverrides, args.status,
args.warning, args.freshenv, args.warningiserror,
Expand Down
16 changes: 7 additions & 9 deletions sphinx/cmd/make_mode.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
blue,
bold,
color_terminal,
nocolor,
colorize_output,
)
from sphinx.util.osutil import rmtree

Expand Down Expand Up @@ -91,14 +91,12 @@ def build_clean(self) -> int:
return 0

def build_help(self) -> None:
if not color_terminal():
nocolor()

print(bold("Sphinx v%s" % sphinx.__display_version__))
print("Please use `make %s' where %s is one of" % ((blue('target'),) * 2))
for osname, bname, description in BUILDERS:
if not osname or os.name == osname:
print(f' {blue(bname.ljust(10))} {description}')
with colorize_output(color_terminal()):
print(bold("Sphinx v%s" % sphinx.__display_version__))
print("Please use `make %s' where %s is one of" % ((blue('target'),) * 2))
for osname, bname, description in BUILDERS:
if not osname or os.name == osname:
print(f' {blue(bname.ljust(10))} {description}')

def build_latexpdf(self) -> int:
if self.run_generic_build('latex') > 0:
Expand Down
115 changes: 60 additions & 55 deletions sphinx/cmd/quickstart.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
bold,
color_terminal,
colorize,
nocolor,
colorize_output,
red,
)
from sphinx.util.osutil import ensuredir
Expand Down Expand Up @@ -558,65 +558,70 @@ def main(argv: Sequence[str] = (), /) -> int:
locale.setlocale(locale.LC_ALL, '')
sphinx.locale.init_console()

if not color_terminal():
nocolor()
with colorize_output(color_terminal()):

# parse options
parser = get_parser()
try:
args = parser.parse_args(argv or sys.argv[1:])
except SystemExit as err:
return err.code # type: ignore[return-value]
# parse options
parser = get_parser()
try:
args = parser.parse_args(argv or sys.argv[1:])
except SystemExit as err:
return err.code # type: ignore[return-value]

d = vars(args)
# delete None or False value
d = {k: v for k, v in d.items() if v is not None}
d = vars(args)
# delete None or False value
d = {k: v for k, v in d.items() if v is not None}

# handle use of CSV-style extension values
d.setdefault('extensions', [])
for ext in d['extensions'][:]:
if ',' in ext:
d['extensions'].remove(ext)
d['extensions'].extend(ext.split(','))

try:
if 'quiet' in d:
if not {'project', 'author'}.issubset(d):
print(__('"quiet" is specified, but any of "project" or '
'"author" is not specified.'))
return 1

if {'quiet', 'project', 'author'}.issubset(d):
# quiet mode with all required params satisfied, use default
d.setdefault('version', '')
d.setdefault('release', d['version'])
d2 = DEFAULTS.copy()
d2.update(d)
d = d2

if not valid_dir(d):
print()
print(bold(__('Error: specified path is not a directory, or sphinx'
' files already exist.')))
print(__('sphinx-quickstart only generate into a empty directory.'
' Please specify a new root path.'))
return 1
else:
ask_user(d)
except (KeyboardInterrupt, EOFError):
print()
print('[Interrupted.]')
return 130 # 128 + SIGINT
# handle use of CSV-style extension values
d.setdefault('extensions', [])
for ext in d['extensions'][:]:
if ',' in ext:
d['extensions'].remove(ext)
d['extensions'].extend(ext.split(','))

for variable in d.get('variables', []):
try:
name, value = variable.split('=')
d[name] = value
except ValueError:
print(__('Invalid template variable: %s') % variable)

generate(d, overwrite=False, templatedir=args.templatedir)
return 0
if 'quiet' in d:
if not {'project', 'author'}.issubset(d):
print(
__('"quiet" is specified, but any of "project" or '
'"author" is not specified.'),
)
return 1

if {'quiet', 'project', 'author'}.issubset(d):
# quiet mode with all required params satisfied, use default
d.setdefault('version', '')
d.setdefault('release', d['version'])
d2 = DEFAULTS.copy()
d2.update(d)
d = d2

if not valid_dir(d):
print()
print(
bold(__('Error: specified path is not a directory, or sphinx'
' files already exist.')),
)
print(
__('sphinx-quickstart only generate into a empty directory.'
' Please specify a new root path.'),
)
return 1
else:
ask_user(d)
except (KeyboardInterrupt, EOFError):
print()
print('[Interrupted.]')
return 130 # 128 + SIGINT

for variable in d.get('variables', []):
try:
name, value = variable.split('=')
d[name] = value
except ValueError:
print(__('Invalid template variable: %s') % variable)

generate(d, overwrite=False, templatedir=args.templatedir)
return 0


if __name__ == '__main__':
Expand Down
12 changes: 11 additions & 1 deletion sphinx/testing/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import pytest

from sphinx.testing.util import SphinxTestApp, SphinxTestAppWrapperForSkipBuilding
from sphinx.util.console import colorize_output

if TYPE_CHECKING:
from collections.abc import Callable, Generator
Expand Down Expand Up @@ -166,6 +167,13 @@ def app(
shared_result.store(test_params['shared_result'], app_)


@pytest.fixture()
def no_colored_output() -> Generator[None, None, None]: # NoQA: PT004
"""Disable colors for output streams."""
with colorize_output(False):
yield


@pytest.fixture()
def status(app: SphinxTestApp) -> StringIO:
"""
Expand All @@ -183,7 +191,9 @@ def warning(app: SphinxTestApp) -> StringIO:


@pytest.fixture()
def make_app(test_params: dict, monkeypatch: Any) -> Generator[Callable, None, None]:
def make_app(
test_params: dict, monkeypatch: Any, no_colored_output: None,
) -> Generator[Callable, None, None]:
"""
Provides make_app function to initialize SphinxTestApp instance.
if you want to initialize 'app' in your test function. please use this
Expand Down
23 changes: 22 additions & 1 deletion sphinx/util/console.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
import re
import shutil
import sys
from contextlib import contextmanager
from typing import Generator

try:
# check if colorama is installed to support color on Windows
Expand Down Expand Up @@ -49,10 +51,14 @@ def term_width_line(text: str) -> str:


def color_terminal() -> bool:
"""Return True if console output should be colored.

This is determined by checking environment variables,
platform, and terminal type.
"""
if 'NO_COLOR' in os.environ:
return False
if sys.platform == 'win32' and colorama is not None:
colorama.init()
return True
if 'FORCE_COLOR' in os.environ:
return True
Expand All @@ -75,9 +81,24 @@ def nocolor() -> None:


def coloron() -> None:
if sys.platform == 'win32' and colorama is not None:
colorama.init()
codes.update(_orig_codes)


@contextmanager
def colorize_output(on: bool) -> Generator[None, None, None]:
"""Temporarily enable or disable colorized output streams."""
global codes
current_codes = codes.copy()
if on:
coloron()
else:
nocolor()
yield
Comment on lines +94 to +98
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the docstring: should there be toggle actions both before and after the yield statement?

Suggested change
if on:
coloron()
else:
nocolor()
yield
(coloron if on else nocolor)()
yield
(nocolor if on else coloron)()

...or something neater (example for illustration purposes, not necessarily good code).

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe toggle is the wrong word then; it should just turn on/off colorization, on entering the context, then return it to the initial state on exiting the context

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little wary of thread-safety / potential timing issues related to the codes object, but thanks for resolving my concern about the content of this function. The code as you have it is OK with me 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I'm not a fan of any mutable global variables, I wish there wasn't so many in docutils/sphinx 😅

codes = current_codes


def colorize(name: str, text: str, input_mode: bool = False) -> str:
def escseq(name: str) -> str:
# Wrap escape sequence with ``\1`` and ``\2`` to let readline know
Expand Down
18 changes: 7 additions & 11 deletions tests/test_quickstart.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,17 @@

from sphinx import application
from sphinx.cmd import quickstart as qs
from sphinx.util.console import coloron, nocolor
from sphinx.util.console import colorize_output

warnfile = StringIO()


def setup_module():
nocolor()
@pytest.fixture(scope='module', autouse=True)
def _setup_module():
real_input = input
with colorize_output(False):
yield
qs.term_input = real_input


def mock_input(answers, needanswer=False):
Expand All @@ -34,14 +38,6 @@ def input_(prompt):
return input_


real_input = input


def teardown_module():
qs.term_input = real_input
coloron()


def test_do_prompt():
answers = {
'Q2': 'v2',
Expand Down