Skip to content

Conversation

@jiasli
Copy link
Member

@jiasli jiasli commented Dec 10, 2020

Description

Support style with color.

For a complete demo, see src/azure-cli/azure/cli/command_modules/util/custom.py and run az demo style.

@jiasli jiasli requested review from houk-ms and zhoxing-ms December 10, 2020 05:14
@yonzhan
Copy link
Collaborator

yonzhan commented Dec 10, 2020

Style

@yonzhan yonzhan requested review from evelyn-ys and jsntcy December 10, 2020 05:56
@yonzhan yonzhan added this to the S180 milestone Dec 10, 2020
@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jiasli jiasli changed the base branch from style to dev December 10, 2020 12:38
Copy link
Contributor

@houk-ms houk-ms left a comment

Choose a reason for hiding this comment

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

Good job ~

Can we also provide a simple developer doc later (may in another pr)?

# https://python-prompt-toolkit.readthedocs.io/en/stable/pages/printing_text.html#style-text-tuples
formatted_parts = []
for text in styled_text:
formatted_parts.append(THEME[text[0]] + text[1])
Copy link
Contributor

Choose a reason for hiding this comment

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

key checking for THEME? Or add some error handling logics here?

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 catch. Added checks and tests as requested.

short-summary: Upgrade Azure CLI and extensions
"""

helps['demo'] = """
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious is this a convention to add a demo command here? The command is convenient for developers but also visible to users right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just curious is this a convention to add a demo command here?

No. We don't have such convention. The only existing usage of this pattern is az self-test:

g.command('self-test', 'check_cli', deprecate_info=g.deprecate(hide=True))

The command is convenient for developers but also visible to users right?

It is not visible to the user, as the parent command group has been marked as hide=True:

with self.command_group('demo', deprecate_info=g.deprecate(hide=True)) as g:
g.custom_command('style', 'demo_style')

}


def print_styled_text(styled, file=sys.stderr):
Copy link
Contributor

Choose a reason for hiding this comment

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

May I ask why the default value of the file parameter is sys.stderr instead of sys.stdout?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only the JSON output should go to stdout and the user may pipe it to downstream commands. Per the description of stderr:

https://en.wikipedia.org/wiki/Standard_streams#sys.stderr

This is similar to sys.stdout because it also prints directly to the Console. But the difference is that can be used to print Exceptions and Error messages and also info/debugging comments. (Which is why it is called Standard Error). This can be very useful when the stdout is used to write data instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, got it~

Copy link
Member Author

Choose a reason for hiding this comment

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

As for az next, it is only used interactively, so the stream doesn't really matter as everything is printed to the screen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

@jiasli
Copy link
Member Author

jiasli commented Dec 11, 2020

Can we also provide a simple developer doc later (may in another pr)?

Of course. I will add the doc once this feature is stabilized.

Copy link
Contributor

@zhoxing-ms zhoxing-ms left a comment

Choose a reason for hiding this comment

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

LGTM!
Maybe we can also encapsulate a util to reuse some common pattern prompts in the future, such as: TASK DONE: Style.SUCCESS + '(✓) Done:' + Style.PRIMARY + ...

@jiasli
Copy link
Member Author

jiasli commented Dec 11, 2020

LGTM!
Maybe we can also encapsulate a util to reuse some common pattern prompts in the future, such as: TASK DONE: Style.SUCCESS + '(✓) Done:' + Style.PRIMARY + ...

Nice suggestion. Let's do this until there is some real usage so that the design/quality can be verified.

@jiasli jiasli marked this pull request as ready for review December 11, 2020 04:41
PRIMARY = "primary"
SECONDARY = "secondary"
IMPORTANT = "important"
ACTION = "action" # name TBD
Copy link
Member Author

Choose a reason for hiding this comment

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

@chenlomis, what style name should we use for light blue?

Choose a reason for hiding this comment

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

Hi there
Am slightly confused
Did you mean finding an internal short name for the the bright blue?
https://devdivdesignguide.azurewebsites.net/command-line-interface/color-guidelines-for-command-line-interface/#action-colors

Copy link
Contributor

@zhoxing-ms zhoxing-ms left a comment

Choose a reason for hiding this comment

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

LGTM

@jiasli jiasli merged commit 821c013 into Azure:dev Dec 11, 2020
@jiasli jiasli added the style label Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants