Skip to content

Conversation

@xfz11
Copy link
Contributor

@xfz11 xfz11 commented Apr 9, 2021

Description

When the enum validation fails, error message would show the allowed values to improve user experience
Before:
image
After:
image

The case in which the candiate list is shown along with the best match candidate:
image

Testing Guide

History Notes

[Component Name 1] BREAKING CHANGE: az command a: Make some customer-facing breaking change.
[Component Name 2] az command b: Add some customer-facing feature.


This checklist is used to make sure that common guidelines for a pull request are followed.

error_msg = "{prog}: '{value}' is not a valid value for '{param}'.".format(
prog=self.prog, value=value, param=parameter)
error_msg = "{prog}: '{value}' is not a valid value for '{param}'. Allowed values: {choice}.".format(
prog=self.prog, value=value, param=parameter, choice=', '.join(sorted([str(x) for x in action.choices])))
Copy link
Member

Choose a reason for hiding this comment

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

Do we need sorted? Shall we respect the order in swagger definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The order in error message is better to be consistent with the order in help info

Copy link
Member

Choose a reason for hiding this comment

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

I personally prefer to keep the order because some choices may be more frequently used.

parameter = action.option_strings[0] if action.option_strings else action.dest
error_msg = "{prog}: '{value}' is not a valid value for '{param}'.".format(
prog=self.prog, value=value, param=parameter)
error_msg = "{prog}: '{value}' is not a valid value for '{param}'. Allowed values: {choice}.".format(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
error_msg = "{prog}: '{value}' is not a valid value for '{param}'. Allowed values: {choice}.".format(
error_msg = "{prog}: '{value}' is not a valid value for '{param}'. Allowed values: {choices}.".format(

@houk-ms houk-ms changed the title add allowed values information when enum validation fails [Core] Add allowed values information when enum validation fails Apr 9, 2021
@houk-ms
Copy link
Contributor

houk-ms commented Apr 9, 2021

@jiasli for awareness. I remember we discussed this before, any plans for this feature in Knack?

@houk-ms
Copy link
Contributor

houk-ms commented Apr 9, 2021

@xfz11 Can you help paste some creenshots in PR description about what the chanegs will look like before and after?

The cases we need to consider include

  1. When the canidate list is very long
  2. When the candiate list is shown along with the best match candidate

@jiasli
Copy link
Member

jiasli commented Apr 9, 2021

The knack implementation is microsoft/knack#244 and have been released in v0.8.1. I have no strong opinion in CLI's behavior and will defer that to @houk-ms.

@houk-ms
Copy link
Contributor

houk-ms commented Apr 9, 2021

@Juliehzl Do you have concerns about it if we decide to display all the possbile candidates?

@houk-ms houk-ms changed the title [Core] Add allowed values information when enum validation fails [Core] Display allowed values in error message when enum validation fails Apr 9, 2021
@houk-ms houk-ms added the Core CLI core infrastructure label Apr 9, 2021
@xfz11 xfz11 merged commit d78a2b9 into Azure:dev Apr 9, 2021
fengzhou-msft pushed a commit to fengzhou-msft/azure-cli that referenced this pull request Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Core CLI core infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants