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

[Core] Knack adoption #12604

Merged
merged 9 commits into from
Mar 20, 2020
Merged

[Core] Knack adoption #12604

merged 9 commits into from
Mar 20, 2020

Conversation

jiasli
Copy link
Member

@jiasli jiasli commented Mar 16, 2020

History Notes:

[Core] PREVIEW: Allow disabling color by setting AZURE_CORE_NO_COLOR environment variable to True or [core] no_color=True config (#12601)
[Core] PREVIEW: Add --only-show-errors global argument to mute all warning, info and debug output. It can also be enabled by setting AZURE_CORE_ONLY_SHOW_ERRORS environment variable to True or [core] only_show_errors=True config (#12544)
[Core] PREVIEW: Add experimental tag to command groups, commands and arguments (#12543)
{Core} Move yaml output to Knack (#12603)


This PR applies changes of microsoft/knack#181.

@yonzhan
Copy link
Collaborator

yonzhan commented Mar 16, 2020

add to S167

@@ -162,7 +166,8 @@ def show_help(self, cli_name, nouns, parser, is_group):
AzCliHelp.update_examples(help_file)
self._print_detailed_help(cli_name, help_file)

print(SURVEY_PROMPT)
from colorama import Fore, Style
Copy link
Member

Choose a reason for hiding this comment

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

from colorama import Fore, Style [](start = 8, length = 32)

suggest to add some description in PR about colorama changes, I cannot understand the difference between colorama.init(autoreset=True) and use from colorama import Fore, Style

Copy link
Member Author

Choose a reason for hiding this comment

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

This is really a long store about terminal coloring. You may check the colorama doc https://pypi.org/project/colorama/ for its usage. colorama.init(autoreset=True) logic has been moved to Knack.

This line is redundant. Let's remove it.

SURVEY_PROMPT = Fore.YELLOW + Style.BRIGHT + 'Please let us know how we are doing: ' + Fore.BLUE \
+ 'https://aka.ms/clihats' + Style.RESET_ALL
SURVEY_PROMPT = 'Please let us know how we are doing: https://aka.ms/clihats'
SURVEY_PROMPT_COLOR = Fore.YELLOW + Style.BRIGHT + 'Please let us know how we are doing: ' + Fore.BLUE \
Copy link
Member

Choose a reason for hiding this comment

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

you can add some snapshot in PR description for this kind of ux change

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice idea!

Originally with color:

image

After running $Env:AZURE_CORE_NO_COLOR='true'. It becomes

image

Copy link
Member

@qianwens qianwens left a comment

Choose a reason for hiding this comment

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

:shipit:

@haroldrandom
Copy link
Contributor

Could resolve conflict and errors first?

@jiasli
Copy link
Member Author

jiasli commented Mar 20, 2020

Could resolve conflict and errors first?

This is because Knack is not released and the dependency is missing. Could you please take a look at microsoft/knack#181?

jiasli added 4 commits March 20, 2020 13:45
# Conflicts:
#	src/azure-cli-core/azure/cli/core/extension/operations.py
@jiasli jiasli merged commit decf463 into dev Mar 20, 2020
@jiasli jiasli deleted the knack-adoption branch March 20, 2020 10:21
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.

5 participants