Skip to content

Conversation

@jiasli
Copy link
Member

@jiasli jiasli commented Jan 30, 2023

Related command
az feedback

Description
Stop including error messages in the feedback body.

@yonzhan yonzhan requested a review from dcaro January 30, 2023 11:05
@yonzhan yonzhan added this to the Jan 2023 (2023-02-07) milestone Jan 30, 2023
@yonzhan
Copy link
Collaborator

yonzhan commented Jan 30, 2023

Az Feedback refinement to fix an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_Put here the error message you received. Make sure to redact sensitive information, such as personally identifiable information (PII), user name, password, credential, etc._
_Insert here the error message you have received. Make sure to redact any sensitive information, such as user name, password, credential, subscription id, etc._

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 think PII is a well-known term. 🙂 https://www.dol.gov/general/ppii

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 choose the word "put" because we already use it in the following section.

- _Put any pre-requisite steps here..._

As for "have", do we really have to use present perfect tense? Simple past tense "received" feels good to me. No need to emphasize the "have received" status.

Also, "any" only serves as an emphasis, so it seems a little bit redundant. But, I am not a native speaker, so may be wrong.

dcaro
dcaro previously approved these changes Jan 30, 2023
Copy link
Contributor

@dcaro dcaro left a comment

Choose a reason for hiding this comment

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

overall lgtm, just a suggestion to improve the message.

Copy link
Contributor

@dbradish-microsoft dbradish-microsoft left a comment

Choose a reason for hiding this comment

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

I do like @dcaro's rewrite, but I simplified it some more. There isn't anything wrong with using PII, but what is similar for our readers and language translators?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
identifiable information (PII), user name, password, credential, subscription ID, etc.
_Insert here the error message you have received. Make sure to remove all sensitive information, such as user name, password, credential, subscription ID, etc._

Copy link
Member Author

@jiasli jiasli Feb 1, 2023

Choose a reason for hiding this comment

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

This line is surrounded by ```, so _ won't work here as italic. I am also wondering which character we should use as a placeholder.

Copy link
Contributor

@dbradish-microsoft dbradish-microsoft left a comment

Choose a reason for hiding this comment

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

@jiasli , I see you have kept "Insert" vs your preferred "put". Maybe "paste" would be even better, but I'm fine with whatever you use.

@jiasli
Copy link
Member Author

jiasli commented Feb 3, 2023

I see you have kept "Insert" vs your preferred "put". Maybe "paste" would be even better, but I'm fine with whatever you use.

Good idea. Changed as suggested.

@jiasli jiasli marked this pull request as ready for review February 3, 2023 02:22
@jiasli jiasli merged commit 353a2f7 into Azure:dev Feb 3, 2023
@jiasli jiasli deleted the feedback branch February 3, 2023 02:59
avgale pushed a commit to avgale/azure-cli that referenced this pull request Aug 24, 2023
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