Skip to content

Conversation

@kairu-ms
Copy link
Contributor

@kairu-ms kairu-ms commented Feb 23, 2021

Description

This PR is for configurable CLI supporting. The core are modified in the following tree aspects:

  • Reconstruction AzCommandGroup
  • Add placeholder decorators for properties used in command definition
  • Decorate those properties defined in cli code

Reconstruction AzCommandGroup

  • Add add_cli_command function in AzCommandsLoader, which supports to register a command in command table by CommandOperation. Reduce the usage of _cli_command fucntion.
  • Replace default default_command_handler default_arguments_loader and default_description_loader in _cli_command by CommandOperation
  • Replace _cli_generic_update_command by GenericUpdateCommandOperation
  • Replace _cli_wait_command by WaitCommandOperation
  • Replace _cli_show_command by ShowCommandOperation

Placeholder Decorators

  • action
    • cls_action_wrapper
    • cls_action_factory_wrapper
  • arg_type
    • register_arg_type
    • arg_type_factory_wrapper
  • client_factory
    • func_client_factory_wrapper
  • completer
    • func_completer_wrapper
    • completer_factory_wrapper
  • exception_handler
    • func_exception_handler_wrapper
  • resource_type
    • register_custom_resource_type
  • transformer
    • func_transformer_wrapper
  • type_converter
    • func_type_converter_wrapper
    • func_type_converter_factory_wrapper
  • validator
    • func_validator_wrapper
    • validator_factory_wrapper

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.

@kairu-ms kairu-ms self-assigned this Feb 23, 2021
@kairu-ms kairu-ms added this to the S184 milestone Feb 23, 2021
@yonzhan
Copy link
Collaborator

yonzhan commented Feb 23, 2021

Core

Copy link
Member

Choose a reason for hiding this comment

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

The word Operation seems redundant to me and makes the functionality difficult to understand. BaseCommand should suffice in my opinion.

Comment on lines 8 to 9
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, decorators like these are too intrusive. They extract information from Azure CLI dynamically during runtime.

If I were you, I would statically parse source code files like _params.py with regex or string operation.

Comment on lines -411 to -413
Copy link
Member

@jiasli jiasli Feb 25, 2021

Choose a reason for hiding this comment

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

I agree and like this refactoring. The original code is obviously abusing closure and makes the code difficult to read. #Resolved

Copy link
Member

@jiasli jiasli Feb 25, 2021

Choose a reason for hiding this comment

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

But still, even in closure, the values can be retrieved using inspect.getclosurevars(func). #WontFix

Comment on lines +28 to +33
Copy link
Member

@jiasli jiasli Feb 25, 2021

Choose a reason for hiding this comment

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

The outer function name can be retrieved using __qualname__ following PEP 3155:

import inspect


def get_printer(msg):
    def printer():
        print(msg)
    return printer


p = get_printer("Hello")
print(p.__qualname__)
print(inspect.getclosurevars(p))

Output:

get_printer.<locals>.printer
ClosureVars(nonlocals={'msg': 'Hello'}, globals={}, builtins={'print': <built-in function print>}, unbound=set())

This can eliminate the necessity of these decorators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your advice. This is a good solution to retrieve data from outer function. But I still prefer decorators because them build a layer between command definitions and linked functions in architecture level, which can not only solve questions above but also bring more flexibility.

@kairu-ms kairu-ms force-pushed the configurable-cli-stage-1 branch from fbac6aa to c7cb205 Compare March 1, 2021 03:22
@kairu-ms kairu-ms force-pushed the configurable-cli-stage-1 branch from fbda11b to ac2e4dc Compare March 3, 2021 08:51
@kairu-ms
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@kairu-ms kairu-ms force-pushed the configurable-cli-stage-1 branch from ac2e4dc to a008d20 Compare March 19, 2021 08:17
@yonzhan yonzhan modified the milestones: S184, S185 Mar 20, 2021
@kairu-ms kairu-ms force-pushed the configurable-cli-stage-1 branch from a008d20 to bcf4148 Compare March 22, 2021 08:40
@kairu-ms kairu-ms closed this Mar 23, 2021
@kairu-ms
Copy link
Contributor Author

Break down to small PRs:

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.

3 participants