Skip to content

Conversation

@fengzhou-msft
Copy link
Member

@fengzhou-msft fengzhou-msft commented Apr 15, 2021

Description

This PR consolidates changes in #17474 and also has the changes to only allow prefix match for help commands during dynamic extension install to resolve #17696.

Testing Guide

az account -s sub_id set: not trigger extension install
az devops --organization org project list: not trigger extension install
az devops: not trigger extension install
az devops -h: trigger extension install
az devops project list --organization org: trigger extension install
az devops project list: trigger extension install

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.

@jiasli
Copy link
Member

jiasli commented Apr 16, 2021

I thought of a better place to trigger extension installation rather than argparse._check_value. argparse._check_value is already too deep.

How about we check whether the command is in the command_table or command_group_table after they are full-loaded:

# No module found from the index. Load all command modules and extensions
logger.debug("Loading all modules and extensions")
_update_command_table_from_modules(args)
ext_suppressions = _get_extension_suppressions(self.loaders)
# We always load extensions even if the appropriate module has been loaded
# as an extension could override the commands already loaded.
_update_command_table_from_extensions(ext_suppressions)
logger.debug("Loaded %d groups, %d commands.", len(self.command_group_table), len(self.command_table))

This will be very easy with the extraction of extension installation logic from _check_value (#17474).

@jiasli
Copy link
Member

jiasli commented Apr 16, 2021

As for the behavior of argparse, it is expected that -s is skipped during parsing and marked as unrecognized later.

We can repro with

import argparse

# create the top-level parser
parser = argparse.ArgumentParser(prog='PROG')
subparsers = parser.add_subparsers(help='sub-command help')

# create the parser for the "a" command
parser_a = subparsers.add_parser('a', help='a help')
parser_a.add_argument('--bar', type=int, help='bar help')

print(parser.parse_args('--bar a'.split()))
usage: PROG [-h] {a} ...
PROG: error: unrecognized arguments: --bar

So checking whether the command is valid is much easier than altering the behavior of argparse.

@fengzhou-msft
Copy link
Member Author

I thought of a better place to trigger extension installation rather than argparse._check_value. argparse._check_value is alrady too deep.

How about we check whether the command is in the command_table or command_group_table after they are full-loaded:

# No module found from the index. Load all command modules and extensions
logger.debug("Loading all modules and extensions")
_update_command_table_from_modules(args)
ext_suppressions = _get_extension_suppressions(self.loaders)
# We always load extensions even if the appropriate module has been loaded
# as an extension could override the commands already loaded.
_update_command_table_from_extensions(ext_suppressions)
logger.debug("Loaded %d groups, %d commands.", len(self.command_group_table), len(self.command_table))

This will be very easy with the extraction of extension installation logic from _check_value (#17474).

Thanks for the suggestion. I will check long-term solutions later.

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