Fix Choice(enum) auto complete not working

[berndbohmeier]

Autocomplete on choices with enums was not working as it didn't use the same logic to normalize it's string value.
I added a test to show the wrong behavior.

However, when I use the same normalize function I see another test failing, and I wanted to discuss. As from my understanding it is allowed to overwrite the normalize method, so I think we have to use it here, in case it is changed to custom behaviour, as whatever is autocompleted should be a valid input. But this means that default behavior would be that we autocomplete for example Fo to foo if the options are [Foo, Bar] with case insensitive, which fails an existing test and could be seen as a breaking change. Please advice, thank you.

fixes #3015

[Rowlando13]

Can you pull down 8.2.2 and see if this is fixed. It is yanked due so you will have to specify it. I think #3004 might fix this.

[berndbohmeier]

Is 8.2.2 newer than main? I added a test case here to show the bug on main.
Which branch do I have to pull to get the latest (potentially unreleased) changes?

[Rowlando13]

Did not look too closely just saw the title. Looks like it was not covered by #3004. Can you please rebase onto stable.

[berndbohmeier]

Thank you, I will rebase it next week, I currently don't have my laptop with me.
Do you have input on the question I had and the currently failing test here?
It seems there is an existing test that ensures we are not changing the case on auto complete, but when I use the existing normalize function we do, so it is breaking this test. As users can overwrite the normalize function from my understanding I think we have to call it though.
Maybe you can give input if I can just change this test and it is an acceptable change for you, or if we have to find another way without changing this behavior.

[berndbohmeier]

@Rowlando13 I rebase on master and fixed the test in the way that I think make the most sense.
The more "correct" solution I think would be to remove the change to lowercase in the normalize function, as in my view this is wrong. But this might be seen as a breaking change? Waiting on your input for this.

[Rowlando13]

It does look like you fix breaks case sensitivity which does not work. I am not sure what the correct normalization path is. If you are able to figure it out, just ping me and I will take a look again.

[berndbohmeier]

Did you take the time to fully understand my PR and what is changing?
At the moment the only thing that will change is that something like cmd F will be autocompleted to cmd foo, which is a change I understand, as at the moment it would be autocompleted to cmd Foo, but it consistent with how currently other places work that use the normalize function. Considering that you can overwrite the normalize function I think this is the only way to fix this, unless you want to change the default normalize function, which would be a breaking change in other places.

Considering that currently autocomplete for options that use a normalize function or an enum is completely broken and not usable at all I think this is the right way forward.

[davidism]

Did you take the time to fully understand my PR and what is changing?

Are there no click developers that could also implement a fix or give some input here?

I understand that you're frustrated that you don't seem to be able to adequately communicate your intention to the maintainer. However, writing like this is unacceptable. If you're not being understood, take a step back until you're ready to approach your explanation again without resorting to implying that the maintainer doesn't know what they're doing.

[davidism]

The behavior around case insensitivity and normalization is complicated, and it can be hard to dive back in and reason about it. This is true of a lot of features in Click, see our current work on flags and defaults. This change makes sense, but it's not immediately obvious.

At first, seeing a test change the behavior from returning a case-sensitive display looks incorrect. If the choice is marked insensitive, then it's reasonable to think the option should be displayed as-is but matched insensitively, and that the test was correct as-is.

However, after looking through more of the code to understand our past decisions, it looks like we're applying the transformation for the metavar and help text as well, so those will display normalized when case-insensitive matching is enabled. Therefore, it does make sense to apply the normalization here as well.

[berndbohmeier]

I understand that you're frustrated that you don't seem to be able to adequately communicate your intention to the maintainer. However, writing like this is unacceptable. If you're not being understood, take a step back until you're ready to approach your explanation again without resorting to implying that the maintainer doesn't know what they're doing.

Apologies if my tone was not right.

[berndbohmeier]

The behavior around case insensitivity and normalization is complicated, and it can be hard to dive back in and reason about it. This is true of a lot of features in Click, see our current work on flags and defaults. This change makes sense, but it's not immediately obvious.

At first, seeing a test change the behavior from returning a case-sensitive display looks incorrect. If the choice is marked insensitive, then it's reasonable to think the option should be displayed as-is but matched insensitively, and that the test was correct as-is.

However, after looking through more of the code to understand our past decisions, it looks like we're applying the transformation for the metavar and help text as well, so those will display normalized when case-insensitive matching is enabled. Therefore, it does make sense to apply the normalization here as well.

Fully agree with this

[Rowlando13]

Thanks! @davidism. Also thank you @berndbohmeier for taking a step back and considering tone. To give a little more context, the team is currently working on another very large PR which will fix a lot of things for users and I did not want to pull anyone away.

[Rowlando13]

If you are curious it's #3030.

[berndbohmeier]

Okay, thank you for the context.