Skip to content

Conversation

@fralik
Copy link

@fralik fralik commented Mar 2, 2021

  • If user provided an invalid value for an argument with a list of choices, then the error message will not indicate this. Rather it will indicate that cli command group is invalid. This is a quick fix and I am not sure that it doesn't break Knack. From my tests, this fix is better than removing overridden _check_value method.

Fixes #236

Vadim Frolov added 2 commits March 2, 2021 21:33
- If user provided an invalid value for an argument with a list of choices, then the error message will not indicate this. Rather it will indicate that cli command group is invalid. This is a quick fix and I am not sure that it doesn't break Knack. From my tests, this fix is better than removing overridden _check_value method.
…rror messages

- This seems to be a better fix for the previous commit.
@ghost
Copy link

ghost commented Mar 2, 2021

CLA assistant check
All CLA requirements met.

Base automatically changed from master to dev March 12, 2021 07:02
@jiasli
Copy link
Member

jiasli commented Mar 18, 2021

@fralik, Thanks a lot for the contribution and this is a nice finding!

The corresponding fix in Azure CLI is done by Azure/azure-cli#6636. The change is later backported to knack by #146, but the logic for invalid argument value handling got lost during the backporting.

As the logic of _check_value in Azur CLI has become rather complex due to addition of new features (Azure/azure-cli#16254, Azure/azure-cli#16257), we need more time investigating and writing tests.

@jiasli
Copy link
Member

jiasli commented Mar 26, 2021

I have created a new PR #244 based on your commits so that you can still get credit and become a contributor. Please kindly see if the new PR satisfies your needs. Thanks.

@jiasli jiasli closed this Mar 26, 2021
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.

Misleading error message for invalid choice arguments

2 participants