[Color] Detect isatty when enabling color #209

jiasli · 2020-05-13T12:51:11Z

This PR is an improvement to #171.

Only enable color if all of the below conditions stand:

Color is not explicitly disabled

AZURE_CORE_NO_COLOR and this config shouldn't be set:

[core]
no_color = yes

`stdout` is a TTY

Otherwise (enable color while stdout is redirected), combined with head command, colorama will fail with BrokenPipeError: [Errno 32] Broken pipe (Azure/azure-cli#13413):

$ az --version | head --lines=1
azure-cli                          2.5.1
The command failed with an unexpected error. Here is the traceback:

[Errno 32] Broken pipe
Traceback (most recent call last):
  File "/opt/az/lib/python3.6/site-packages/knack/cli.py", line 207, in invoke
    self.show_version()
  File "/opt/az/lib/python3.6/site-packages/azure/cli/core/__init__.py", line 146, in show_version
    print('\n' + (SURVEY_PROMPT_COLOR if self.enable_color else SURVEY_PROMPT))
  File "/opt/az/lib/python3.6/site-packages/colorama/ansitowin32.py", line 41, in write
    self.__convertor.write(text)
  File "/opt/az/lib/python3.6/site-packages/colorama/ansitowin32.py", line 162, in write
    self.write_and_convert(text)
  File "/opt/az/lib/python3.6/site-packages/colorama/ansitowin32.py", line 187, in write_and_convert
    self.write_plain_text(text, cursor, start)
  File "/opt/az/lib/python3.6/site-packages/colorama/ansitowin32.py", line 196, in write_plain_text
    self.wrapped.flush()
BrokenPipeError: [Errno 32] Broken pipe

To open an issue, please run: 'az feedback'
Exception ignored in: <_io.TextIOWrapper name='<stdout>' mode='w' encoding='UTF-8'>
BrokenPipeError: [Errno 32] Broken pipe

With this RP, color sequences are not printed or sent to downstream command.

$ az --version | head --lines=1
azure-cli                          2.5.1

Also, if color is written to stderr, the terminal color can't revert back (Azure/azure-cli#6080, Azure/azure-cli#8483, Azure/azure-cli#11671, Azure/azure-cli#12084, Azure/azure-cli#13979) :

This is due to colorama issue tartley/colorama#200. In order for color to be reset correctly, the Style.RESET_ALL control sequence must be sent to both stdout and stderr. However, colorama can't send Style.RESET_ALL to stderr correctly, causing the terminal stuck at the previous color:

stdout:  Fore.CYAN  -->    Style.RESET_ALL
stderr:  Fore.RED   -->   (Style.RESET_ALL)   <<< colorama can't send this

`stderr` is a TTY

Otherwise (enable color while stderr is redirected),

az foo --verbose 2>err.txt

err.txt will have no level information (because there is no color):

az: 'foo' is not in the 'az' command group. See 'az --help'. If the command is from an extension, please make sure the corresponding extension is installed. To learn more about extensions, please visit https://docs.microsoft.com/en-us/cli/azure/azure-cli-extensions-overview
command ran in 0.559 seconds.

With this RP, each message will be prefixed with a LEVEL_TAG - ERROR, WARNING, INFO, DEBUG:

ERROR: az: 'foo' is not in the 'az' command group. See 'az --help'. If the command is from an extension, please make sure the corresponding extension is installed. To learn more about extensions, please visit https://docs.microsoft.com/en-us/cli/azure/azure-cli-extensions-overview
INFO: Command ran in 0.917 seconds (init: 0.077, invoke: 0.840)

jiasli · 2020-05-13T12:52:44Z

knack/util.py

+    # Code copied from
+    # https://github.com/tartley/colorama/blob/3d8d48a95de10be25b161c914f274ec6c41d3129/colorama/ansitowin32.py#L43-L53
+    import sys
+    if 'PYCHARM_HOSTED' in os.environ:


PyCharm's integrated terminal is not a tty but it does support color, so detect if the command is run in PyCharm's integrated terminal. If so, enable color.

However, this still has some edge cases (Azure/azure-cli#9903, Azure/azure-sdk-for-python#11362) when colorama is used within PyCharm: tartley/colorama#263.

Even though colorama already has this logic and can strip escape sequences \033[ if the stream is not a TTY, we still need to detect it by ourselves so that we don't print escape sequences when colorama is not initialized. In other words, we shouldn't rely on colorama to do the stripping.

Maybe you can put comment into code.

jiasli · 2020-05-13T12:55:55Z

knack/output.py

    def get_formatter(self, format_type):  # pylint: disable=no-self-use
        # remove color if stdout is not a tty
-        if not sys.stdout.isatty() and format_type == 'jsonc':
+        if not self.cli_ctx.enable_color and format_type == 'jsonc':


Correct the logic to use cli_ctx.enable_color instead of sys.stdout to make the decision about falling back from jsonc to json.

jiasli · 2020-06-19T05:41:59Z

knack/cli.py

+        # As logging is initialized in `invoke`, call `logger.debug` or `logger.info` here won't work.
+        self.init_debug_log = []
+        self.init_info_log = []


Add lists for debug and info logs which can't be printed with logger.debug and logger.info yet, until the logger has been initialized.

mmyyrroonn · 2020-06-19T08:54:18Z

knack/cli.py

        self.only_show_errors = self.config.getboolean('core', 'only_show_errors', fallback=False)
-        self.enable_color = not self.config.getboolean('core', 'no_color', fallback=False)
+
+        # Color is only enabled when all conditions are met:


good comments!

jiasli · 2020-07-07T07:31:20Z

CI was broken due to a breaking change in isort PyCQA/isort#1273:

py36 run-test: commands[2] | pylint knack --rcfile=.pylintrc -r n -d I0013
Traceback (most recent call last):
  File "/home/vsts/work/1/s/.tox/py36/bin/pylint", line 8, in <module>
    sys.exit(run_pylint())
  File "/home/vsts/work/1/s/.tox/py36/lib/python3.6/site-packages/pylint/__init__.py", line 20, in run_pylint
    Run(sys.argv[1:])
  File "/home/vsts/work/1/s/.tox/py36/lib/python3.6/site-packages/pylint/lint.py", line 1628, in __init__
    linter.check(args)
  File "/home/vsts/work/1/s/.tox/py36/lib/python3.6/site-packages/pylint/lint.py", line 943, in check
    self._do_check(files_or_modules)
  File "/home/vsts/work/1/s/.tox/py36/lib/python3.6/site-packages/pylint/lint.py", line 1075, in _do_check
    self.check_astroid_module(ast_node, walker, rawcheckers, tokencheckers)
  File "/home/vsts/work/1/s/.tox/py36/lib/python3.6/site-packages/pylint/lint.py", line 1158, in check_astroid_module
    walker.walk(ast_node)
  File "/home/vsts/work/1/s/.tox/py36/lib/python3.6/site-packages/pylint/utils.py", line 1305, in walk
    cb(astroid)
  File "/home/vsts/work/1/s/.tox/py36/lib/python3.6/site-packages/pylint/checkers/imports.py", line 507, in leave_module
    std_imports, ext_imports, loc_imports = self._check_imports_order(node)
  File "/home/vsts/work/1/s/.tox/py36/lib/python3.6/site-packages/pylint/checkers/imports.py", line 664, in _check_imports_order
    isort_obj = isort.SortImports(
AttributeError: module 'isort' has no attribute 'SortImports'
ERROR: InvocationError for command /home/vsts/work/1/s/.tox/py36/bin/pylint knack --rcfile=.pylintrc -r n -d I0013 (exited with code 1)

Updating Pylint so that isort is pinned (pylint-dev/pylint#2773).

qwordy · 2020-07-08T02:12:14Z

knack/util.py

+
+def isatty(stream):
+    # Code copied from
+    # https://github.com/tartley/colorama/blob/3d8d48a95de10be25b161c914f274ec6c41d3129/colorama/ansitowin32.py#L43-L53


Can code under BSD-3-Clause be used in MIT project?

You should not copy code from BSD to MIT

Refactored to remove unnecessary code.

qwordy

Pay attention to license issue.

jiasli commented May 13, 2020

View reviewed changes

jiasli requested review from arrownj and fengzhou-msft May 13, 2020 13:05

fengzhou-msft approved these changes May 29, 2020

View reviewed changes

jiasli mentioned this pull request Jun 18, 2020

Console color doesn't reset after error when output is redirected Azure/azure-cli#13979

Closed

jiasli force-pushed the detect-tty branch from 85536ed to f8a061b Compare June 19, 2020 03:10

jiasli commented Jun 19, 2020

View reviewed changes

Detect isatty

d75391a

jiasli force-pushed the detect-tty branch from f7da2f2 to d75391a Compare June 19, 2020 08:49

mmyyrroonn reviewed Jun 19, 2020

View reviewed changes

Make print_init_log private

69c24dc

jiasli requested a review from qwordy July 7, 2020 07:02

Merge branch 'master' into detect-tty

ac8e2c2

jiasli force-pushed the detect-tty branch from 5a84295 to ac8e2c2 Compare July 7, 2020 08:46

qwordy reviewed Jul 8, 2020

View reviewed changes

qwordy suggested changes Jul 8, 2020

View reviewed changes

yonzhan assigned jiasli Jul 14, 2020

jiasli added 2 commits December 2, 2020 15:15

Merge branch 'master' into detect-tty

d1cc6f4

Remove util.isatty

6ac1274

qwordy approved these changes Dec 2, 2020

View reviewed changes

jiasli changed the title ~~Detect isatty when enabling color~~ [Color] Detect isatty when enabling color Dec 2, 2020

Update tests

db41a9f

jiasli merged commit 7e458cb into microsoft:master Dec 17, 2020

jiasli deleted the detect-tty branch December 17, 2020 09:08

jiasli mentioned this pull request Jan 20, 2021

Console colors do not always revert back to default Azure/azure-cli#6080

Closed

jiasli mentioned this pull request Aug 12, 2021

Logs from handle_version_update is not shown in --debug mdoe Azure/azure-cli#19201

Open

This was referenced Sep 2, 2021

Init colorama only in Windows legacy terminal #238

Merged

ANSI escape color codes broken on Git Bash in Windows Terminal Azure/azure-cli#19391

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Color] Detect isatty when enabling color #209

[Color] Detect isatty when enabling color #209

Uh oh!

jiasli commented May 13, 2020 •

edited

Loading

Uh oh!

jiasli May 13, 2020 •

edited

Loading

Uh oh!

jiasli May 13, 2020

Uh oh!

qwordy Jul 8, 2020

Uh oh!

jiasli May 13, 2020

Uh oh!

jiasli Jun 19, 2020

Uh oh!

mmyyrroonn Jun 19, 2020

Uh oh!

jiasli commented Jul 7, 2020 •

edited

Loading

Uh oh!

qwordy Jul 8, 2020

Uh oh!

qwordy Jul 8, 2020

Uh oh!

jiasli Dec 2, 2020

Uh oh!

qwordy left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[Color] Detect isatty when enabling color #209

[Color] Detect isatty when enabling color #209

Uh oh!

Conversation

jiasli commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Color is not explicitly disabled

stdout is a TTY

stderr is a TTY

Uh oh!

jiasli May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiasli commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qwordy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jiasli commented May 13, 2020 •

edited

Loading

`stdout` is a TTY

`stderr` is a TTY

jiasli May 13, 2020 •

edited

Loading

jiasli commented Jul 7, 2020 •

edited

Loading