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.

Sign up for GitHub

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

Jump to bottom

Fall back to FORCE_COLOR environment variable if MYPY_FORCE_COLOR is not present #13814

Merged
merged 2 commits into from
Oct 4, 2022
Merged

Fall back to FORCE_COLOR environment variable if MYPY_FORCE_COLOR is not present #13814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e30fc0d
Fall back to FORCE_COLOR environment variable if MYPY_FORCE_COLOR is …
AlexWaygood Oct 4, 2022
5a81b02
Address verbal review: make function return a `bool`
AlexWaygood Oct 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions mypy/dmypy/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from mypy.dmypy_os import alive, kill
from mypy.dmypy_util import DEFAULT_STATUS_FILE, receive
from mypy.ipc import IPCClient, IPCException
from mypy.util import check_python_version, get_terminal_width
from mypy.util import check_python_version, get_terminal_width, should_force_color
from mypy.version import __version__

# Argument parser. Subparsers are tied to action functions by the
Expand Down Expand Up @@ -653,7 +653,7 @@ def request(
args["command"] = command
# Tell the server whether this request was initiated from a human-facing terminal,
# so that it can format the type checking output accordingly.
args["is_tty"] = sys.stdout.isatty() or int(os.getenv("MYPY_FORCE_COLOR", "0")) > 0
args["is_tty"] = sys.stdout.isatty() or should_force_color()
args["terminal_width"] = get_terminal_width()
bdata = json.dumps(args).encode("utf8")
_, name = get_status(status_file)
Expand Down
7 changes: 5 additions & 2 deletions mypy/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,10 @@ def parse_gray_color(cup: bytes) -> str:
return gray


def should_force_color() -> bool:
return bool(int(os.getenv("MYPY_FORCE_COLOR", os.getenv("FORCE_COLOR", "0"))))
Comment on lines +522 to +523
Copy link

Choose a reason for hiding this comment

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

fyi, if FORCE_COLOR is used for a different purpose (e.g. a different python library), it could contain a string, and this would raise an error.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, would be better to just check whether FORCE_COLOR exists, with any value (even 0!).

Copy link
Member Author

@AlexWaygood AlexWaygood Oct 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

(even 0!)

...Really? If somebody puts FORCE_COLOR: 0 in a GitHub Actions workflow, they expect it to force color??

Copy link
Member Author

Choose a reason for hiding this comment

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

(even 0!)

...Really? If somebody puts FORCE_COLOR: 0 in a GitHub Actions workflow, they expect it to force color??

From a review of the Python projects @hugovk linked to in #13806 (comment), it looks like the answer is: yup! tox is the only project on that list that will treat FORCE_COLOR: 0 as indicating that the user does not want color.

Copy link
Member Author

@AlexWaygood AlexWaygood Oct 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for flagging this @nnrepos!

Copy link

Choose a reason for hiding this comment

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

@AlexWaygood thanks, but this is not what i was talking about. i was talking about a possible ValueError in case the variable is not a number (e.g. False).

Copy link
Member

Choose a reason for hiding this comment

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

...Really? If somebody puts FORCE_COLOR: 0 in a GitHub Actions workflow, they expect it to force color??

Yes, that's what NO_COLOR does! 😅

Command-line software which adds ANSI color to its output by default should check for a NO_COLOR environment variable that, when present and not an empty string (regardless of its value), prevents the addition of ANSI color.

https://no-color.org/

Here's how pytest does it:

https://github.com/pytest-dev/pytest/blob/b7d4de1ea983d3c4d3d9e235f63942abc6bfb167/src/_pytest/_io/terminalwriter.py#L26-L37

Copy link
Member Author

@AlexWaygood AlexWaygood Oct 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@AlexWaygood thanks, but this is not what i was talking about. i was talking about a possible ValueError in case the variable is not a number (e.g. False).

Yeah, I understood! #13853 means that you won't get a ValueError for FORCE_COLOR if you provide a value for that variable that can't be passed to the int() constructor. (It preserves the behaviour for MYPY_FORCE_COLOR that was present prior to this PR, however, since that's longstanding behaviour, and "fixing" it might be a breaking change in some ways.)

#13853 looks like it might be rejected, though, so this may be a moot point :)



class FancyFormatter:
"""Apply color and bold font to terminal output.

Expand All @@ -531,8 +535,7 @@ def __init__(self, f_out: IO[str], f_err: IO[str], hide_error_codes: bool) -> No
if sys.platform not in ("linux", "darwin", "win32", "emscripten"):
self.dummy_term = True
return
force_color = int(os.getenv("MYPY_FORCE_COLOR", "0"))
if not force_color and (not f_out.isatty() or not f_err.isatty()):
if not should_force_color() and (not f_out.isatty() or not f_err.isatty()):
self.dummy_term = True
return
if sys.platform == "win32":
Expand Down