Repository: pallets/click

Problem statement:
Add coercion/normalization of Choice values
I want my CLI to be case insensitive for certain `Choice` options but I don't want to deal with that variability internally as it has no meaning.  I think an option like `coerce_case` or `normalize_case` would be a reasonable addition.

PR to follow.


Hints:
It appears #887 already added this.
Never mind, this is about what value gets returned, that was about what got matched.
> I don't want to deal with that variability internally as it has no meaning.

Sounds like you should just be handling things in all-lowercase? #887 set things to return the `normed_value` -- so when `case_sensitive=False`, `click.Choice` should always be returning a lowercase string.
(Yes, this should be improved in the documentation for `case_sensitive=False`.)

Reading #1278, it seems this is about allowing `--foo [Apple|Orange]` and always returning title-case `Apple` or `Orange` even if `case_sensitive=False`.

Unless there's some other rationale, I don't think this is well-justified. It adds another parameter to `click.Choice` which is conditional on `case_sensitive=False` and "undoes" part of that behavior -- I think that will be confusing for most users.

I'm not saying there isn't a reason! In #569 I argued that I really have a use-case where case-sensitivity is driven by external constraints. But I'd need to know what the use-case is before agreeing that this should be part of `click`.

[I'm reminded of this comment on the initial request for case_sensitive](https://github.com/pallets/click/issues/569#issuecomment-216515603). If there is a clear use-case here, it's worth considering a generic parameter which changes the `token_normalize_func` on a per-option basis.
@sirosen, yes, #1278 retains the original (not necessarily title) case specified by the choices.  It seems to me this may have been a better approach to begin with as opposed having case insensitivity force the introduction of a third casing (lower as opposed to original choice and user-entered).  But, for backwards compatibility I introduced this as an option with default behavior remaining as it is now.

My actual use case is in https://github.com/altendky/romp where I have choices for platform and Python interpreter (and more, but specifically those).  I stuck a copy of the modified choice in there which I am using for now.

https://github.com/altendky/romp/blob/742d4ac5ff0b918e33002af1f374b8ce6938367a/src/romp/_matrix.py#L11-L18
```python
vm_images = collections.OrderedDict((
    ('Linux', 'ubuntu-16.04'),
    ('macOS', 'macOS-10.13'),
    ('Windows', 'vs2017-win2016'),
))


all_platforms = tuple(vm_images.keys())
```

`all_platforms` is used as a choice list elsewhere.  I could `.lower()` in various places for comparison or create another mapping `lower_platforms_to_display = {platform.lower(): platform for platform in all_platforms}` and use that wherever I want to display the proper case.  Or I could recover the case from the `.lower()`ed result from the Click choice.  Or I could just have one casing that I work with everywhere.

As mentioned above it's not clear to me yet that the present functionality of 'returning' the lower case form is actually good over returning original or user entered.  Is there a reason it is beyond (the sufficient) backwards compatibility?

Thanks for your time and feedback.
Thanks for linking the original/relevant source of this issue. I was kind of guessing this might be a matter of having a source of data where case is important, then being fed into the CLI opts.

I may have to take a break from click for the moment, but will try to circle back and look again later this week -- but I can answer one question:

> As mentioned above it's not clear to me yet that the present functionality of 'returning' the lower case form is actually good over returning original or user entered. Is there a reason it is beyond (the sufficient) backwards compatibility?

Maybe those who reviewed it have a different take, but as the author of #887, no, there's no reason. 😅 

My use case was that I wanted `--foo [apple|orange]` and not to care about `--foo oRanGE`, but I didn't really at the time consider that the choices offered might have actually had important information encoded in the case.

We could argue that changing this behavior would be a backwards incompatible change -- demanding that it be saved for v8.0 -- or we could make the case that, as I noted that "the doc for case_sensitive should be improved", the behavior is unspecified. I would be okay with saying `case_sensitive=False` returns the choice value as specified, not lowercased, and calling it a fix for v7.1.
(i.e. `case_sensitive=False` has a bug in v7.0, in that it _incorrectly_ returns a lowercased value instead of the matching choice value)

Realistically, most callers using `case_sensitive=True` are probably, like me, passing in a list of lowercase strings and would see no change.

Making the new behavior in #1278 the only behavior would almost definitely make `click` better to use long-term. Changing the default behavior strikes me as better than adding another option with conditional/nuanced behavior.

I want to just change it in v7.1 and field any complaints. But if we do that and someone _is_ relying on the lowercasing and gets broken, I'm sure I'll catch some grief for taking that stance...
@sirosen, I can certainly make another PR to implement the 'fix' rather than the 'feature addition' in #1278.  For the people banking on the forced lowercase result I guess they would get whatever misc failures in their code and the fix would be `option_value = option_value.lower()` (or, `.casefold()` perhaps if py3).  An option to do that in Click itself would of course be easy but it seems it wouldn't add much value and would probably just be a generic post-proc to allow for choosing lower vs casefold vs... ??  who knows.  I suppose the value in having it in Click is in reuse of defined options (I do this a good bit, `x_option = click.Option('--x')` then `@x_option` on multiple commands).  Or, perhaps Click already has such a feature?  I honestly am not familiar with the possible modifiers.

No rush on this on my behalf.  I've got my workaround in place for now and can remove it whenever.
> Or, perhaps Click already has such a feature? I honestly am not familiar with the possible modifiers.

The best path on that sort of thing is to define a specialized decorator which applies your option with desired params. The `callback` parameter lets you do post-processing pretty cleanly:

```python
def common_options(cmd):
    def _lower_callback(ctx, param, value):
        if value is None:
            return value
        else:
            return value.lower()

    cmd = click.option('--x', callback=_lower_callback)(cmd)
    cmd = click.option('--foo', callback=_lower_callback)(cmd)
    return cmd
```

---

If you're comfortable with letting this wait around a little, I'd like to hear someone else's input (e.g. @davidism if he can spare the time) on whether or not it's okay to simply change the behavior and say it was unspecified in 7.0 and will be specified in 7.1 .
Just to be clear, the new proposal is to make `case_sensistive=True` mean "match case insensitive, but return exact value", and `False` would be "match and return exact"?
@davidism, I believe so.  Where 'exact' refers to 'as passed to `click.Choice()` in code' not 'as written on the command line'.
Seems fine for 7.1, along with clarifying the documentation of the option, it would be good to add an example of normalizing to lowercase if there's not one already.
@davdism, for the additional example, you are referring to `callback=_lower_callback` which would allow recovery of the existing 7.0 behavior regarding `case_sensitive=False`?

Note that 7.0 returns the normed value not the exact value passed to `click.Choice()`.  So we have three values: 'exact', 'normed', and 'normed+lowered'.  I haven't personally used the normalization system.  Do we want to restrict this change to only correcting the result for `case_sensitive=False` and make it return the 'normed' value?  Or to universally return the 'exact' value passed to the `click.Choice()` call thus also changing behavior for normalized values?  The latter seems more complete and easy to clearly document.

To have the list handy, and in case I missed something, here is the documentation for `click.Choice()` that I found.  None of them seem to clarify what will be passed to the command.

* http://click.palletsprojects.com/en/7.x/api/#click.Choice
  * https://github.com/pallets/click/blob/baf01249d2bb662390f66a5574a264d723862b8e/click/types.py#L130-L140
* http://click.palletsprojects.com/en/7.x/options/#choice-opts
  * https://github.com/pallets/click/blob/baf01249d2bb662390f66a5574a264d723862b8e/docs/options.rst#L311-L344
* http://click.palletsprojects.com/en/7.x/advanced/?highlight=choice#token-normalization
  * https://github.com/pallets/click/blob/baf01249d2bb662390f66a5574a264d723862b8e/docs/advanced.rst#L120-L128
* http://click.palletsprojects.com/en/7.x/bashcomplete/?highlight=choice#what-it-completes
  * https://github.com/pallets/click/blob/baf01249d2bb662390f66a5574a264d723862b8e/docs/bashcomplete.rst#L18-L24
I think the best thing to do for the examples is to write a command which takes a choice like `Apple|Orange|Banana` and then show how it can be updated to `case_insensitive=False` to also accept `apple`, `APPLE`, and other spellings. Show that `Apple` is the string passed to the command function regardless -- and note that it is specified by the option definition, not what's passed on the command-line.

The token normalize function and use of `callback` on `Option`s are both probably out of scope for this -- at least, I wouldn't add them to the examples unless lots of people show up asking questions about how they interact with this specific case.

> Do we want to restrict this change to only correcting the result for `case_sensitive=False` and make it return the 'normed' value? Or to universally return the 'exact' value passed to the `click.Choice()` call thus also changing behavior for normalized values?

To clarify, you're distinguishing
- the value passed to `click.Choice`
vs
- the value passed to `click.Choice`, passed through `ctx.token_normalize_func`, but without `str.lower()`
?

I think the easiest behavior to explain is that it returns the original string passed to `click.Choice`, not the normalized-but-not-lowercased version.

---

Also, something we haven't discussed is that `case_sensitive=False` will interact with this new behavior in an interesting way if multiple choices are different spellings of the same value.
With `click.Choice(['apple', 'APPLE'], case_sensitive=False)` -- it now will matter which one of these two choices `aPPle` matches.

I think it's fine for such behavior to remain undefined, but, if possible, I'd like to take the first or last matching choice in a consistent way.
Yes, that is the value I'm referring to.

Or maybe raise an exception on creation of a non-unique case-insensitive list?  `if len(set(choice.lower() for choice in choices)) < len(choices): raise...`  It always looked funny having the optimized direct check against the original choices at the beginning.  It allowed things to match without enforcing the normalization.
I think these are the two open questions.

Questions:
- Should the pre-callback value always be the same as was passed to `click.Choice()`?
  - We already agreed that `case_sensitive=False` should be corrected to not modify the value
  - Presently `ctx.token_normalize_func` modifies the value, should that behavior be changed?
- What should be done with `click.Choice(['apple', 'APPLE'], case_sensitive=False)`?
  - Undefined?
  - Raise an exception?
  - Out of scope for this ticket?

> Presently ctx.token_normalize_func modifies the value, should that behavior be changed?

I think so. My rationale is based on looking at the diff for #887 . Prior to that, the choice value which was matched, without passing through `ctx.token_normalize_func`, would be returned by `click.Choice`.

So *not* applying `ctx.token_normalize_func` is the most similar behavior to click 6.x and below.

> What should be done with click.Choice(['apple', 'APPLE'], case_sensitive=False)?

I'm fine with any of the options you put forth.
They all make sense on the grounds that "If the user isn't sure what he/she is asking for, how could Click possibly know?" :)

Patch:
diff --git a/click/types.py b/click/types.py
--- a/click/types.py
+++ b/click/types.py
@@ -133,6 +133,10 @@ class Choice(ParamType):
     You should only pass a list or tuple of choices. Other iterables
     (like generators) may lead to surprising results.
 
+    The resulting value will always be one of the originally passed choices
+    regardless of ``case_sensitive`` or any ``ctx.token_normalize_func``
+    being specified.
+
     See :ref:`choice-opts` for an example.
 
     :param case_sensitive: Set to false to make choices case
@@ -152,29 +156,34 @@ def get_missing_message(self, param):
         return 'Choose from:\n\t%s.' % ',\n\t'.join(self.choices)
 
     def convert(self, value, param, ctx):
-        # Exact match
-        if value in self.choices:
-            return value
-
         # Match through normalization and case sensitivity
         # first do token_normalize_func, then lowercase
         # preserve original `value` to produce an accurate message in
         # `self.fail`
         normed_value = value
-        normed_choices = self.choices
+        normed_choices = {choice: choice for choice in self.choices}
 
-        if ctx is not None and \
-           ctx.token_normalize_func is not None:
+        if ctx is not None and ctx.token_normalize_func is not None:
             normed_value = ctx.token_normalize_func(value)
-            normed_choices = [ctx.token_normalize_func(choice) for choice in
-                              self.choices]
+            normed_choices = {
+                ctx.token_normalize_func(normed_choice): original
+                for normed_choice, original in normed_choices.items()
+            }
 
         if not self.case_sensitive:
-            normed_value = normed_value.lower()
-            normed_choices = [choice.lower() for choice in normed_choices]
+            if PY2:
+                lower = str.lower
+            else:
+                lower = str.casefold
+
+            normed_value = lower(normed_value)
+            normed_choices = {
+                lower(normed_choice): original
+                for normed_choice, original in normed_choices.items()
+            }
 
         if normed_value in normed_choices:
-            return normed_value
+            return normed_choices[normed_value]
 
         self.fail('invalid choice: %s. (choose from %s)' %
                   (value, ', '.join(self.choices)), param, ctx)


Test patch:
diff --git a/tests/test_normalization.py b/tests/test_normalization.py
--- a/tests/test_normalization.py
+++ b/tests/test_normalization.py
@@ -20,7 +20,7 @@ def test_choice_normalization(runner):
     @click.command(context_settings=CONTEXT_SETTINGS)
     @click.option('--choice', type=click.Choice(['Foo', 'Bar']))
     def cli(choice):
-        click.echo('Foo')
+        click.echo(choice)
 
     result = runner.invoke(cli, ['--CHOICE', 'FOO'])
     assert result.output == 'Foo\n'
diff --git a/tests/test_options.py b/tests/test_options.py
--- a/tests/test_options.py
+++ b/tests/test_options.py
@@ -335,12 +335,15 @@ def cmd(foo):
 
     result = runner.invoke(cmd, ['--foo', 'apple'])
     assert result.exit_code == 0
+    assert result.output == 'Apple\n'
 
     result = runner.invoke(cmd, ['--foo', 'oRANGe'])
     assert result.exit_code == 0
+    assert result.output == 'Orange\n'
 
     result = runner.invoke(cmd, ['--foo', 'Apple'])
     assert result.exit_code == 0
+    assert result.output == 'Apple\n'
 
     @click.command()
     @click.option('--foo', type=click.Choice(['Orange', 'Apple']))
@@ -357,6 +360,18 @@ def cmd2(foo):
     assert result.exit_code == 0
 
 
+def test_case_insensitive_choice_returned_exactly(runner):
+    @click.command()
+    @click.option('--foo', type=click.Choice(
+        ['Orange', 'Apple'], case_sensitive=False))
+    def cmd(foo):
+        click.echo(foo)
+
+    result = runner.invoke(cmd, ['--foo', 'apple'])
+    assert result.exit_code == 0
+    assert result.output == 'Apple\n'
+
+
 def test_multiline_help(runner):
     @click.command()
     @click.option('--foo', help="""