CLICommandsLoader.create_command overrides the argument "arguments_loader"

[niander]

Issue here:

knack/knack/commands.py

Line 265 in dc7b7d8

kwargs['arguments_loader'] = arguments_loader

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]

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]

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]

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.