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

Allow disabling color #171

Merged
merged 7 commits into from
Mar 16, 2020
Merged

Allow disabling color #171

merged 7 commits into from
Mar 16, 2020

Conversation

jiasli
Copy link
Member

@jiasli jiasli commented Dec 3, 2019

Fixes #170, #169. Also reported in Azure/azure-cli#6080 and causes side effect of Azure/azure-cli#9903.

If stdout is redirected and CLI displays a colored output, the color in the console can't revert back to the default because Colorama only resets color in stdout, but not stderr (tracked at tartley/colorama#200, tartley/colorama#218). This is affecting pip as well: pypa/pip#6354 and pip uses --no-color to disable coloring pypa/pip#4739. Azure CLI will add this option too.

Without proper a fix from Colorama, Knack can now

  • Disable color via {CLI}_CORE_NO_COLOR environment variable.
  • Use config
    [core]
    no_color=True
    

The PR removes Colorma from

  1. output: output.py which replaces out_file with a Colorma stream and generates colorized output like jsonc
  2. log: log.py which replaces self.stream with a Colorma stream and generates colorized logs
  3. help: help.py which initializes Colorama for help message
  4. deprecation/preview warning: invocation.py which initializes Colorama for deprecation and preview warning messages and util.py which generates colorized warning for StatusTag

self.stream.isatty() is removed because Colorama has its own mechanism to detect tty and treat PyCharm as a tty: https://github.com/tartley/colorama/blob/405b982f39efeaf697d3e2a6b90f9b913b38e97d/colorama/ansitowin32.py#L45

To test,

  1. Clone Knack to the local machine
  2. Pull from my branch
  3. Activate the virtual env of Azure CLI and run
pip install --editable <path_to_knack>

# disable color
# cmd
set {CLI}_CORE_NO_COLOR=1
# bash
export {CLI}_CORE_NO_COLOR=1
# pwsh
$Env:{CLI}_CORE_NO_COLOR=1

# enable color
set {CLI}_CORE_NO_COLOR=

# test output color
az vm list --output jsonc

# test log color
az vm list --debug

# test help color
az vm create -h

# test deprecation warning color
az vm image accept-terms

@Juliehzl
Copy link
Member

Juliehzl commented Dec 3, 2019

@jiasli Should we consider upgrading knack version for the change?

@jiasli
Copy link
Member Author

jiasli commented Dec 3, 2019

Yes. But let's test it first. We will bump the version when other PRs are merged.

@yungezz
Copy link

yungezz commented Dec 3, 2019

the change will disable color output from az cli totally?

@jiasli
Copy link
Member Author

jiasli commented Dec 3, 2019

Yes, Colorama won't be initialized with KNACK_NO_COLOR set to 1.

@jsntcy
Copy link
Member

jsntcy commented Dec 10, 2019

  • So if the user need disable color, does he/she need call 'set KNACK_NO_COLOR=1'?
  • If user 'set KNACK_NO_COLOR=1'. is the behavior compatible as before for correct scenarios?

@jiasli
Copy link
Member Author

jiasli commented Jan 14, 2020

The user can either call set KNACK_NO_COLOR=1 or set environment variable for the user or the system.

If the user runs set KNACK_NO_COLOR=1, only color is disabled with no change in the behavior, except that warnings will be prefixed with the level as a substitution for coloring, like ERROR for red 🟥, WARNING for yellow 🟨, INFO for green 🟩 and DEBUG for blue 🟦.

@jiasli jiasli changed the base branch from master to dev March 16, 2020 11:23
@jiasli jiasli merged commit 82875fb into microsoft:dev Mar 16, 2020
@jiasli jiasli mentioned this pull request Mar 16, 2020
jiasli added a commit that referenced this pull request Mar 20, 2020
* Allow disabling color (#171)

* Support --only-show-errors (#179)

* Add experimental tag (#180)
@jiasli jiasli deleted the no-color branch March 24, 2020 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow disabling color
4 participants