search

microsoft / knack

Knack - A Python command line interface framework
https://pypi.python.org/pypi/knack
MIT License
348 stars 95 forks source link

CLICommandsLoader.create_command overrides the argument "arguments_loader" #219

Open niander opened 4 years ago

niander commented 4 years ago

Issue here: https://github.com/microsoft/knack/blob/dc7b7d857ce3c11b2a3155b81f2470b3b579e7c9/knack/commands.py#L265

The whole command registration process suggests that it is possible to give a custom arguments loader when calling the command method in a CommandGroup context. However, this parameter is overwritten with something else in the CLICommandsLoader.create_command method making impossible to pass a custom arguments_loader.

The same is true for the argument description_loader.

What was the behavior expected here? Shouldn't the arguments_loader from kwargs supersede the default one?

jiasli commented 4 years ago

Azure CLI core does support custom argument_loader and description_loader:

https://github.com/Azure/azure-cli/blob/68ae330b5ac0b5e3975f698ef5bd76838f9cf883/src/azure-cli-core/azure/cli/core/__init__.py#L786-L798

        def default_arguments_loader():
            ...

        def default_description_loader():
            ...

        kwargs['arguments_loader'] = argument_loader or default_arguments_loader
        kwargs['description_loader'] = description_loader or default_description_loader

However, may I know the motivation to make CLICommandsLoader.create_command accept custom arguments_loader and description_loader. Could you give more description about how what are trying to achieve?

niander commented 4 years ago

Hello, I am just reporting what seems to be a bug or design flaw. If Azure CLI core support custom argument_loader and description_loader why knack does not? As I said, the fact it accepts a custom argument_loader suggests it should use it instead of the default one. Is there a particular reason for this not being the case? Actually, it seems Azure CLI is fixing it while knack should be the one fixing.

If this is in fact a bug I can make a PR if you wanted.

niander commented 3 years ago

Hello. Just following up on this. I am happy to fix that in knack but please let me know if this is a bug or just wrong documentation. The command method in CommandGroup clearly states that arguments_loader and description_loader are valid arguments but these are ignored later on as I showed above.

While I am aware Azure CLI core does not have this problem, knack is still a valid project isn't it? We use knack and not Azure CLI Core. Thank you.