WIP: Require an automatic parameter name to be a Python identifier

[kdeldycke]

This PR changes the behavior of automatic naming:

• An argument must produce a valid Python identifier. 0-file, foo.bar and foo bar are now refused.
• expose_value=False no longer bypasses that check, on both arguments and options.
• An argument or option requires at least one declaration that can't be empty. click.argument(expose_value=False) and click.option(expose_value=False) used to produce a "" parameter.
• An awkward explicit option name is normalized: click.option("--in-file", "Input_File") names its parameter input_file.

What does not change:

• The transformation itself: every - becomes _ and the result is lower cased.
• An exposed option still raise if not producing a Python identifier, like click.option("--0-file").

How to migrate:

• Give an option an explicit name: click.option("--0-file", "zero_file").
• Rename an argument and pass metavar to keep the old display: click.argument("zero_file", metavar="0-FILE").

This PR shares the majority of the tests from #3866 which lockdown all other edge-cases and canonical naming behavior.

This PR is waiting for #3866 to be merged into the 8.x release cycle to be rebased on it.

Context

It is in the same vein as my previous PR at #3808 , and is also based on a collection of edge-cases I accumulated over the years. It also covers platform differences between Windows and Unix-like (see: #2483).

Note: this PR started as simple test coverage expansion but ended up implementing sanitizing fixes.

[davidism]

Both of these seem like bugs. I feel like the automatic names should always be normalized and pass str.isidentifier.

[kdeldycke]

Both of these seem like bugs. I feel like the automatic names should always be normalized and pass str.isidentifier.

OK cool, that was also my feeling. We don't want special treatments of the name of parameters, whatever their kind (options or arguments). I cannot find any reasons with the transformation should be different between the two kinds. So in the end yes, str.isidentifier should be the minimal check common to the two.

[kdeldycke]

Both of these seem like bugs. I feel like the automatic names should always be normalized and pass str.isidentifier.

@davidism Done. I did not pushed the normalization too much.

So there is one bothering difference between Argument and Option: the latter makes click.option("--in-file", "Input_File") names its parameter Input_File, while Argument forces it to input_file. Should I align both? I would tend to yes.

[kdeldycke]

I just pushed the alignment and normalization of naming a bit more and updated the differences in the body of that PR.

[kdeldycke]

OK these were my final edits. You can review this PR.

[davidism]

This needs to be targeted at 9.0 and listed high up in the changelog, the name normalization is a breaking change. Along with the keyword check I mention below, this could be significantly disruptive. We should probably issue a deprecation warning and accept names that are currently accepted, before applying the new rules.

It might be good for the TypeError to give explicit instructions: Argument {decls} tried to use 'name' but it is not a valid Python identifier. Add a valid name to the parameter declaration.

Overall I think the docs are a little too verbose about this. Giving a few examples and linking to isidentifier should be enough.

You bring up a good point about keywords. I think the intention of this code was clear: to only accept names that can be used in signatures rather than kwargs. We can also check that the name isn't a keyword with https://docs.python.org/3/library/keyword.html. I bet from and in are pretty common option names though, perhaps this is not worth it?

[Rowlando13]

I agree with @davidism. The docs are too verbose and these changes should go through normal deprecation process. Probably a best course of action is to make a pr against stable that has whatever tests you think we should have to lock in existing behavior. Then can merge stable into main. And you can make the new pr against main with the deprecations.

[kdeldycke]

Ahah agreed 100% about the verbosity of the docs. They accumulates all my changes and discovery through the process of producing this PR. It was a way for me to highlight how clunky the ID derivation is at the moment. My thinking being that if I can write the docs easily and make them flow naturally, it probably means the underlying concept and implementation is clean.

Ok for a 9.0.0 target then.

What I will do:

• keep targeting this for a clean break for 9.0.0
• propose another PR for 8.5.1/8.6.0 with proper detection of the edge-cases, deprecation warnings and migration documentation so the future break has time to diffuse in the ecosystem

[kdeldycke]

You bring up a good point about keywords. I think the intention of this code was clear: to only accept names that can be used in signatures rather than kwargs. We can also check that the name isn't a keyword with https://docs.python.org/3/library/keyword.html. I bet from and in are pretty common option names though, perhaps this is not worth it?

Problem is the list of Python keywords would prevent some options like --import or --from. Which is a hard constraint to impose a developer of a CLI. I prefer to not enforce checks too much here.

[kdeldycke]

OK so I think I am done with this PR and it now implements the ideal target behavior for 9.0.0. I will now create in the next few days a sibling PR for 8.5.1/8.6.0 that detects the changes and emits deprecation warnings. If I discover new stuff while producing this other PR I might bring some changes here.