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

Init colorama only in Windows legacy terminal #238

Merged
merged 3 commits into from
Mar 22, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
24 changes: 13 additions & 11 deletions knack/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
from .completion import CLICompletion
from .output import OutputProducer
from .log import CLILogging, get_logger
from .util import CLIError
from .util import CLIError, is_modern_terminal
from .config import CLIConfig
from .query import CLIQuery
from .events import EVENT_CLI_PRE_EXECUTE, EVENT_CLI_SUCCESSFUL_EXECUTE, EVENT_CLI_POST_EXECUTE
Expand Down Expand Up @@ -101,6 +101,8 @@ def __init__(self,

self.only_show_errors = self.config.getboolean('core', 'only_show_errors', fallback=False)
self.enable_color = self._should_enable_color()
# Init colorama only in Windows legacy terminal
self._should_init_colorama = self.enable_color and os.name == 'nt' and not is_modern_terminal()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not (self.enable_color or _KNACK_TEST_FORCE_ENABLE_COLOR) and ...? Does that mean knack test always use colorama no matter it's modern terminal or legacy terminal?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not (self.enable_color or _KNACK_TEST_FORCE_ENABLE_COLOR) and ...?

The action scope of mocked _KNACK_TEST_FORCE_ENABLE_COLOR is CLI.invoke. If _KNACK_TEST_FORCE_ENABLE_COLOR is used in CLI.__invoke__, CLI.invoke won't be affected and won't have colorama enabled.

knack/tests/util.py

Lines 36 to 37 in cd59031

with mock.patch("knack.cli._KNACK_TEST_FORCE_ENABLE_COLOR", True):
func(self)

Does that mean knack test always use colorama no matter it's modern terminal or legacy terminal?

Yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Dropped _KNACK_TEST_FORCE_ENABLE_COLOR. Now strip control sequences by knack itself:

knack/tests/util.py

Lines 57 to 58 in e5e0529

def _remove_control_sequence(string):
return re.sub(r'\x1b[^m]+m', '', string)

Copy link
Contributor

Choose a reason for hiding this comment

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

huge work😂


@staticmethod
def _should_show_version(args):
Expand Down Expand Up @@ -207,7 +209,8 @@ def invoke(self, args, initial_invocation_data=None, out_file=None):
exit_code = 0
try:
out_file = out_file or self.out_file
if out_file is sys.stdout and self.enable_color or _KNACK_TEST_FORCE_ENABLE_COLOR:
if out_file is sys.stdout and self._should_init_colorama or _KNACK_TEST_FORCE_ENABLE_COLOR:
self.init_debug_log.append("Init colorama.")
import colorama
colorama.init()
# point out_file to the new sys.stdout which is overwritten by colorama
Expand Down Expand Up @@ -250,7 +253,7 @@ def invoke(self, args, initial_invocation_data=None, out_file=None):
finally:
self.raise_event(EVENT_CLI_POST_EXECUTE)

if self.enable_color or _KNACK_TEST_FORCE_ENABLE_COLOR:
if self._should_init_colorama or _KNACK_TEST_FORCE_ENABLE_COLOR:
import colorama
colorama.deinit()

Expand All @@ -274,14 +277,13 @@ def _should_enable_color(self):
self.init_debug_log.append("Color is disabled by config.")
return False

if 'PYCHARM_HOSTED' in os.environ:
if sys.stdout == sys.__stdout__ and sys.stderr == sys.__stderr__:
self.init_debug_log.append("Enable color in PyCharm.")
return True
else:
if sys.stdout.isatty() and sys.stderr.isatty() and self.out_file is sys.stdout:
self.init_debug_log.append("Enable color in terminal.")
return True
if sys.stdout.isatty() and sys.stderr.isatty() and self.out_file is sys.stdout:
self.init_debug_log.append("Enable color in terminal.")
return True

if 'PYCHARM_HOSTED' in os.environ and sys.stdout == sys.__stdout__ and sys.stderr == sys.__stderr__:
self.init_debug_log.append("Enable color in PyCharm.")
return True

self.init_debug_log.append("Cannot enable color.")
return False
21 changes: 21 additions & 0 deletions knack/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,3 +142,24 @@ def todict(obj, post_processor=None): # pylint: disable=too-many-return-stateme
if not callable(v) and not k.startswith('_')}
return post_processor(obj, result) if post_processor else result
return obj


def is_modern_terminal():
"""Detect whether the current terminal is a modern terminal that supports Unicode and
Console Virtual Terminal Sequences.

Currently, these terminals can be detected:
- VS Code terminal
- PyCharm
- Windows Terminal
"""
# VS Code: https://github.com/microsoft/vscode/pull/30346
if os.environ.get('TERM_PROGRAM', '').lower() == 'vscode':
return True
# PyCharm: https://youtrack.jetbrains.com/issue/PY-4853
if 'PYCHARM_HOSTED' in os.environ:
return True
# Windows Terminal: https://github.com/microsoft/terminal/issues/1040
if 'WT_SESSION' in os.environ:
return True
return False
15 changes: 13 additions & 2 deletions tests/test_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@
# Licensed under the MIT License. See License.txt in the project root for license information.
# --------------------------------------------------------------------------------------------

from collections import namedtuple
import unittest
from collections import namedtuple
from datetime import date, time, datetime
from unittest import mock

from knack.util import todict, to_snake_case
from knack.util import todict, to_snake_case, is_modern_terminal


class TestUtils(unittest.TestCase):
Expand Down Expand Up @@ -87,6 +88,16 @@ def test_to_snake_case_already_snake(self):
actual = to_snake_case(the_input)
self.assertEqual(expected, actual)

def test_is_modern_terminal(self):
with mock.patch.dict("os.environ", clear=True):
self.assertEqual(is_modern_terminal(), False)
with mock.patch.dict("os.environ", TERM_PROGRAM='vscode'):
self.assertEqual(is_modern_terminal(), True)
with mock.patch.dict("os.environ", PYCHARM_HOSTED='1'):
self.assertEqual(is_modern_terminal(), True)
with mock.patch.dict("os.environ", WT_SESSION='c25cb945-246a-49e5-b37a-1e4b6671b916'):
self.assertEqual(is_modern_terminal(), True)


if __name__ == '__main__':
unittest.main()