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

{Aladdin} Add v0 hook to AzCliCommandParser class. #12889

Merged
merged 18 commits into from
Apr 14, 2020
Merged

{Aladdin} Add v0 hook to AzCliCommandParser class. #12889

merged 18 commits into from
Apr 14, 2020

Conversation

christopher-o-toole
Copy link
Contributor

@christopher-o-toole christopher-o-toole commented Apr 6, 2020

Description of PR (Mandatory)
Add hook to allow extensions to make suggestions on parser failures.

Failure Types

Failure Description Command to Reproduce Parse Error Handler
Command is not in a command group az list-av AzCliCommandParser._check_value
Arguments are required az vm AzCliCommandParser.error
Expected one argument az login --username AzCliCommandParser.error
Unrecognized parameter az login --list AzCliCommandParser.error
Invalid JMESPath Query az vm list --query ".id" AzCliCommandParser.error

@msftclas
Copy link

msftclas commented Apr 6, 2020

CLA assistant check
All CLA requirements met.

@yonzhan yonzhan added this to the S168 milestone Apr 6, 2020
@yonzhan
Copy link
Collaborator

yonzhan commented Apr 6, 2020

add to S168

src/azure-cli-core/azure/cli/core/parser.py Outdated Show resolved Hide resolved
src/azure-cli-core/azure/cli/core/parser.py Show resolved Hide resolved
src/azure-cli-core/azure/cli/core/parser.py Outdated Show resolved Hide resolved
src/azure-cli-core/azure/cli/core/parser.py Outdated Show resolved Hide resolved
src/azure-cli-core/azure/cli/core/parser.py Outdated Show resolved Hide resolved
src/azure-cli-core/azure/cli/core/parser.py Outdated Show resolved Hide resolved
@@ -52,6 +52,10 @@ def _get_completions(self, comp_words, cword_prefix, cword_prequote, last_wordbr
class AzCliCommandParser(CLICommandParser):
"""ArgumentParser implementation specialized for the Azure CLI utility."""

@staticmethod
def recommendation_provider(version, command, parameters, extension):
pass
Copy link
Member

Choose a reason for hiding this comment

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

I noticed there are many cases where recommendation can be triggered, please list all of these cases in the PR description. Including how to trigger them.

Use logger.debug to log the args and make sure they are provided as expected.

We can schedule a meeting to further discuss this design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will put a more detailed description of some of the most common cases where recommendations are triggered soon.

I'd like to schedule a meeting with you if you have time. I'll contact you on Teams for details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added better logging through logger.debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added table of different command failures. Will add additional details sometime tonight.

.vscode/launch.json Outdated Show resolved Hide resolved
.vscode/launch.json Outdated Show resolved Hide resolved
src/azure-cli-core/azure/cli/core/parser.py Outdated Show resolved Hide resolved
src/azure-cli-core/azure/cli/core/parser.py Outdated Show resolved Hide resolved
Copy link
Member

@jiasli jiasli left a comment

Choose a reason for hiding this comment

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

LGTM. @arrownj, please share your advice as it is using some changes from #12741.

Copy link
Contributor

@arrownj arrownj left a comment

Choose a reason for hiding this comment

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

looks good to me.

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.

6 participants