pallets / click

Python composable command line interface toolkit
https://click.palletsprojects.com
BSD 3-Clause "New" or "Revised" License
15.56k stars 1.39k forks source link

`flag_value` is passed through `ParamType.convert` #2012

Open jgadling opened 3 years ago

jgadling commented 3 years ago

In upgrading from click 7.1.2 to 8.0.1, our click-based CLI started failing with type errors. We have click options whose default values are classes, and as of 8.0.1 they're being converted to strings.

This script is able to reproduce the behavior change:

import click

class Class1:
    pass

class Class2:
    pass

@click.command()
@click.option("--cls1", "config_cls", flag_value=Class1, default=True)
@click.option("--cls2", "config_cls", flag_value=Class2)
def test(config_cls):
    print(config_cls)
    print(type(config_cls))

if __name__ == "__main__":
    test()

Behavior with click 7.1.2:

# pip freeze | grep click
click==7.1.2
# python test.py
<class '__main__.Class1'>
<class 'type'>

Behavior with 8.0.1:

# pip freeze | grep click
click==8.0.1
# python test.py
<class '__main__.Class1'>
<class 'str'>

The type of config_cls should be the class we specified, not 'str'.

Environment:

MHannila commented 3 years ago

This seems to have already appeared in the 8.0.0 version.
Ran into this myself using enum member as the flag_value and managed to get an integer to convert to string as well so I did some testing (with 8.0.1).

A simple workaround solution for most cases seems to be just defining the type, but there are also issues if you have multiple flags for the same argument that are of different types. Then even defining the types doesn't help. That hopefully isn't a problem for many though, because why would you even do that.

from enum import Enum

import click
import pkg_resources

class EnumClass(Enum):
    FOO = 1
    BAR = 2

@click.command()
@click.option("--enum-no-type", flag_value=EnumClass.FOO, default=True)
@click.option("--enum-with-type", flag_value=EnumClass.FOO, type=EnumClass, default=True)
@click.option("--integer", flag_value=1, default=True)
@click.option("--integer-as-int", 'multitype_borked', flag_value=1, default=True)
@click.option("--integer-as-str", 'multitype_borked', flag_value='1')
@click.option("--integer-as-int-with-type", 'multitype_borked2', flag_value=1, type=int, default=True)
@click.option("--integer-as-str-with-type", 'multitype_borked2', flag_value='1', type=str)
def test(**kwargs):
    for name, value in kwargs.items():
        print(name, value, type(value))

if __name__ == '__main__':
    print('click version', pkg_resources.get_distribution('click').version)
    test()
click version 8.0.1
enum_no_type EnumClass.FOO <class 'str'>
enum_with_type EnumClass.FOO <enum 'EnumClass'>
integer 1 <class 'int'>
multitype_borked 1 <class 'str'>
multitype_borked2 1 <class 'str'>

same with 7.1.2 works just as you'd expect

click version 7.1.2
enum_no_type EnumClass.FOO <enum 'EnumClass'>
enum_with_type EnumClass.FOO <enum 'EnumClass'>
integer 1 <class 'int'>
multitype_borked 1 <class 'int'>
multitype_borked2 1 <class 'int'>

Trying to define type with class doesn't work, unsurprisingly

import click

class Class: pass

@click.command()
@click.option("--class-with-type", flag_value=Class, type=Class, default=True)
def test(**kwargs):
    for name, value in kwargs.items():
        print(name, value, type(value))

if __name__ == "__main__":
    test()
Traceback (most recent call last):
  File "/Users/<REDACTED>/Library/Application Support/JetBrains/PyCharm2021.2/scratches/scratch_66.py", line 34, in <module>
    test()
  File "/usr/local/lib/python3.6/dist-packages/click/core.py", line 1137, in __call__
    return self.main(*args, **kwargs)
  File "/usr/local/lib/python3.6/dist-packages/click/core.py", line 1061, in main
    with self.make_context(prog_name, args, **extra) as ctx:
  File "/usr/local/lib/python3.6/dist-packages/click/core.py", line 923, in make_context
    self.parse_args(ctx, args)
  File "/usr/local/lib/python3.6/dist-packages/click/core.py", line 1379, in parse_args
    value, args = param.handle_parse_result(ctx, opts, args)
  File "/usr/local/lib/python3.6/dist-packages/click/core.py", line 2364, in handle_parse_result
    value = self.process_value(ctx, value)
  File "/usr/local/lib/python3.6/dist-packages/click/core.py", line 2320, in process_value
    value = self.type_cast_value(ctx, value)
  File "/usr/local/lib/python3.6/dist-packages/click/core.py", line 2307, in type_cast_value
    return convert(value)
  File "/usr/local/lib/python3.6/dist-packages/click/types.py", line 75, in __call__
    return self.convert(value, param, ctx)
  File "/usr/local/lib/python3.6/dist-packages/click/types.py", line 170, in convert
    return self.func(value)
TypeError: object() takes no parameters
techouse commented 3 years ago

Had the same issue and just adding a type to the click options fixed it for me.

davidism commented 2 years ago

The processing pipeline was updated to be more strict about casing values to types consistently. Click (7 and 8) only detect some basic param types, and fall back to click.STRING otherwise. Since the custom class isn't a recognized type, Click falls back to string, and then applies that type conversion to the value during processing. You can avoid this for now by passing type=click.UNPROCESSED, although I'll see if I can think of a way to handle this type of case.

kleinweby commented 2 years ago

We've stumbled across this same issue. And I'm a bit confused.

Looking at the docu, which states

Changed in version 8.0.1: type is detected from flag_value if given.

I would expect some behavior along the lines of

if type is MISSING:
   type = type(flag_value)

However reading davidism comment and looking at https://github.com/pallets/click/blob/2996d3c11972ad1d71339481f2119495185e7d8d/src/click/types.py#L959

this only hold true, if the autoguessed type is one of str, int, float, bool. If that is the intended behavior I think the documentation should state so, as I find it very surprising.

However I think, in the case of flag_value I would simply expect click not to touch that all and simply pass it back to me.