What is this?

It's the list of excluded params aside from 'self' and 'client', which are used for client_factory.
'generic_wait' had a bug where if the operation signature included 'client', it would not be used for the client_factory (only 'self' would).
generic_update was doing the right thing: https://github.com/Azure/azure-cli/blob/dev/src/azure-cli-core/azure/cli/core/commands/arm.py#L442

if possible, find a better name than the EXCLUDED_NON_CLIENT_PARAMS. It is a bit hard to read and hard to understand

I don't see the simplification here. Previously if you have a custom getter, you would specify the getter name and getter_type=custom_type. This seems like it opens up the potential to have conflicting arguments. What happens if I put getter_type=custom_type and custom_command=False?

The getter_type and setter_type will always take priority.
If getter_type=custom_type, then the custom_type will be used.
This should simplify things since most getter_type/setter_types are custom.
ex. https://github.com/Azure/azure-cli/blob/dev/src/command_modules/azure-cli-eventgrid/azure/cli/command_modules/eventgrid/commands.py#L35-L45

In the example above, 3 lines would be saved for the one generic_update command as there would be no need to redefine the custom_command_type.

Why isn't custom_command just another kwarg?

kwargs are passed along. This ensures that custom_command is only used where it is intended; in the generic_* related methods..

This is the role of the getter_type kwarg. I don't agree with providing multiple ways to do the same thing. This convenience flag only works when a module has a single custom command type. It breaks down (whereas getter_type does not) when custom commands are broken into multiple files (see storage and monitor).

True yes, getter_type can still be used as the default to provide more flexibility. custom_command is a convenience.
This is the same reason why we have the command and custom_command methods, you can still register a custom command using command and specifying a command_type.

Hmm… we should probably chat about this offline.

Is this a minor fix or a breaking change? Most of these changes signify breaking changes, so we can't necessarily rely on our boilerplate text here.

True, thanks

The change needed to this test implies there is a serious breaking change here.

True

Breaking changes should bump the middle number, not the right-most number.

Is there anything actually generic about this? In the case of update, the generic piece are the --set, --add, --remove arguments. In the case of wait, it's the different generic wait conditions.

My preference here would be to expose show_command and custom_show_command which eliminates the need for the custom_command kwarg and neatly parallels the command/custom_command pattern.

I would leave generic update/wait as is.

Hmm, depends on how we define "generic". It's generic in terms of providing a base functionality for all commands registered with it. The signature for the commands are also similar.
It does not, however, provide any additional arguments.

I like the idea of exposing show_command/custom_show_command but it does seem inconsistent if update/wait do not follow the *_command/custom_*_command pattern.

@troydai @yugangw-msft, what do you guys think about the addition of a 'custom_command' kwarg?

Currently, generic_* commands will only make use of the command_type kwarg. custom_command_type, whether defined in the command_loader, or the command_group() is ignored. The custom_command kwarg will refer to the custom_command_type by default, for cases like this.

They are dubbed generic because of the syntax (especially for "generic" update), not because they enforce some common behavior. The add arguments that may not actually work for a given situation (for example, you create a resource with --no-wait and then wait on --deleted... dumb thing to do, but the command can't know).

I do agree that wait and show should use the same mechanisms, because in probably 99% of cases they likely are using the exact same getters. I would rather advocate for this scheme in lieu of the flag:

show_command(…)
custom_show_command(…)
wait_command(…)
custom_wait_command(…)
generic_wait_command(…) # deprecate and phase out
generic_update_command(…) # remains unaltered

The main difference is that show and wait only rely on a single callable, so it is appropriate to say whether that kwarg bundle comes from the normal command_type bundle or the custom_command_type bundle since we do that for 'regular' commands.

However, generic update weaves together 2-3 independent callables to accomplish something. There should only be one default for this, and that default, which fits 90% of the cases is that the setter and getter come from command_type and the custom piece comes from custom_command_type. If you deviate from that pattern, you have to get specific. I'd bet at least half of the usages of generic update that use custom getters and setters could be rewritten to not use them at all. There are a small number of cases where Swagger naming issues necessitate using the name kwarg, but the default still holds for the types.

Having both wait and show use the same custom_command/command pattern sounds good to me as they have the same syntax. I agree that generic_update is much more complex in syntax to, and thus can be grouped apart from, the other two.
I'll leave generic_update as is.

One more thing, is there a need to "deprecate" generic_wait_command as it's only seen by developers? Not sure how deprecating it would look like, and I can replace the method now with this PR.

I'd recommend you get rid of getter type for these since the type is specified by whether wait_command or custom_wait_command is used.

Same. No longer need getter_type.

It seems like rather than having lots of logic like this:

kwargs.get('custom_command_type', None) if custom_command else kwargs.get('command_type', None)

Where you risk a logic error if there's a typo, you could just leverage the source_kwarg argument you removed elsewhere. That way you specify it in fewer places and your logic looks like:

kwargs.get(source_kwarg, None)

You also wouldn't need that big if-else block in _get_operations_tmpl because you would just pass it the source_kwarg directly.

Yes, make this accept source_kwarg (no default) and have show_command pass 'command_type' and custom_show_command pass 'custom_command_type'. It will simplify your code. Ditto for the private wait command.

The problem with passing along source_kwarg is that it does not give any indication what all the possible choices are for it.
Validation also needs to be done on that source_kwarg in the case that a spelling typo is made unless we are relying on this being caught elsewhere (not ideal).

Using a flag gives the user clear information that there are two possible choices and leaves less room for error.
To lessen the <>if custom_command else <> statements, I've added a util method that selects the right source_kwarg that will be utilized in all the cases.

Works for me.

You should alter the signature of custom_wait_command so that the second positional arg is the getter_name without specifying it. Then it looks and works just like custom_show_command and custom_command.

getter_name IS the second postional argument in the signature, similar to the show commands and generic_update. They all(show/update/wait) also allow the use of the argument as a keyword, so something like custom_wait_command('wait', '_server_postgresql_get' is also valid.
I've left the usage of the argument as a keyword as the author of the command did.

Ah, in that case, I'd just recommend removing it so it looks the rest.

You should not delete this docstring. It is used to provide intellisense. Publicly exposed methods should have docstrings, including your new ones :)

This on the other hand does not need a docstring (but there's no harm in leaving it).

You need to preserve the docstring here. You might also try copying the _command docstring to this. You just need to make sure the intellisense still works for the command and I think by doing this, you do away with it.

Can confirm that intellisense does not work(at least in vscode), but for what cases would we try to ensure intellisense is enabled?
We have many publicly exposed methods in our CLI, with very few containing docstrings, command/custom_command were the very rare exceptions.

IMO, intellisense does not seem very important unless we expect people to use the CLI as a library to call methods from.
As there is no harm in them, I can add docstring documentation for the command registration methods if you feel strongly otherwise.

TL/DR: intellisense is important and we should have the docstrings on those methods

We should have docstrings on any public methods we expect command authors to use. So the public methods of command group and argument context should have docstrings to provide intellisense. A classic example is the resource_id method that now lives in msrestazure.tools. This originally was strongly typed, which provided some kind of intellisense. When Johan converted it to just use kwargs, it became impossible to use without pulling up code somewhere to know what you can use. A docstring was added so that the user gets intellisense. This is just as important for these methods that rely heavily on kwargs.

It would be good to stick to one nomenclature--either custom_type or custom_command. My recommendation would actually be is_custom_command since that's more clearly a Boolean. custom_type vs. custom_func_type.... doesn't really make sense why they are different data types.

Works for me.

can we rename to _wait_command or any other w/o the generic? I am a bit sensitive to generic_xxx naming as those are supposed to be public facing. But up to you though since i don't work on basic command infrastructure that much.

if possible, find a better name than the EXCLUDED_NON_CLIENT_PARAMS. It is a bit hard to read and hard to understand

why not name it get_command_type?

great idea, had it as source_kwarg due to the argument name present before I made changes. I'll change it to get_command_type_kwarg, as it returns the keyword, not the command_type itself.