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

[Tag] Allow customizing tag message #223

Merged
merged 1 commit into from
Dec 17, 2020
Merged

Conversation

jiasli
Copy link
Member

@jiasli jiasli commented Oct 30, 2020

A singleton dict status_tag_messages is used to hold configurable StatusTag messages. Developers can override the content of status_tag_messages to customize the help and warning messages.

@jiasli jiasli changed the title {Tag} Allow customizing tag message [Tag] Allow customizing tag message Oct 30, 2020
@@ -50,8 +51,7 @@ def __init__(self, cli_ctx, object_type='', target=None, tag_func=None, message_
"""

def _default_get_message(self):
return "This {} is experimental and not covered by customer support. " \
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed "not covered by customer support", as for Knack there is no customer support at all!

@jiasli jiasli self-assigned this Nov 24, 2020
@jiasli jiasli marked this pull request as ready for review November 24, 2020 03:22
@jiasli jiasli requested review from chenlomis and removed request for arrownj December 9, 2020 02:40
@@ -7,6 +7,7 @@

_EXPERIMENTAL_TAG = '[Experimental]'
_experimental_kwarg = 'experimental_info'
EXPERIMENTAL_MESSAGE = "{} is experimental and under development."
Copy link
Member

Choose a reason for hiding this comment

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

should we let customer know that experimental commands may be changed/removed in a future release?

Choose a reason for hiding this comment

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

Hi Zunli!

Let me chime in a bit...

We actually looped in our researcher and UX writers to rephrase the messaging. Their suggestion is that "under development" is a more succinct expression

Also @jiasli -- a super minor grammar call out (which I just realized)

Can we add an "is" to the sentence? --->

"{} is experimental and is under development."

Seems to be more grammatically accurate

I hope I'm not too late with the comment...

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 message won't be seen in Azure CLI in any way. It will be overridden, like in Azure/azure-cli#15730.

I hope I'm not too late with the comment...

Not too late at all, given @evelyn-ys is concerned about using a module variable.

"{} is experimental and is under development."

To me, the sentence structure is {} is [(experimental) and (under development)], but I am open to this. + @dbradish-microsoft to comment.

Choose a reason for hiding this comment

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

Thanks Delora! @dbradish-microsoft

@@ -300,8 +302,10 @@ def __call__(self, parser, namespace, values, option_string=None):
return ExperimentalArgumentAction

def _get_experimental_arg_message(self):
return "{} '{}' is experimental and not covered by customer support. " \
"Please use with discretion.".format(self.object_type.capitalize(), self.target)
from .experimental import EXPERIMENTAL_MESSAGE
Copy link
Member

Choose a reason for hiding this comment

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

How could developers customize the message in your current design? It seems that you define the message variable with hard-coded message. If I am a developer who want to change the message, I have to re-write the message in knack. Is it by design?

Copy link
Member Author

Choose a reason for hiding this comment

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

A developer can customize the message like Azure/azure-cli#15730. I will add a comment to the variable as well.

Copy link

@chenlomis chenlomis left a comment

Choose a reason for hiding this comment

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

LGTM!
Zunli and Yishi may elaborate/comment better on the actual code changes

@@ -11,6 +11,14 @@

NO_COLOR_VARIABLE_NAME = 'KNACK_NO_COLOR'

# Override these values to customize the status message.
# The message should contain a placeholder indicating the subject (like 'This command group', 'Commend group xxx').
# (A dict is used to avoid the "from A import B" pitfall that creates a copy of the imported B.)
Copy link
Member Author

Choose a reason for hiding this comment

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

@jiasli jiasli merged commit c265fd4 into microsoft:master Dec 17, 2020
@jiasli jiasli deleted the custom-tag-msg branch December 17, 2020 07:29
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.

4 participants