Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wrong type detected from default=tuple when multiple=True #1649

Closed
davidism opened this issue Aug 17, 2020 · 5 comments · Fixed by #1687
Closed

wrong type detected from default=tuple when multiple=True #1649

davidism opened this issue Aug 17, 2020 · 5 comments · Fixed by #1687
Assignees
Labels
bug f:parameters feature: input parameter types
Milestone

Comments

@davidism
Copy link
Member

davidism commented Aug 17, 2020

Expected Behavior

When creating a parameter with multiple=True and default=(1, 2), the type should be detected as INT. Calling get_default should return the default.

import click

opt = click.Option(["-c"], multiple=True, default=(1, 2))
print(opt.type)
ctx = click.Context(click.Command("test"))
value = opt.get_default(ctx)
print(value)
(1, 2)

Actual Behavior

The type is detected as Tuple(INT, INT). This then causes problems when get_default calls type_cast_value.

Traceback (most recent call last):
  File "/home/david/Projects/click/example.py", line 5, in <module>
    value = opt.get_default(ctx)
  File "/home/david/Projects/click/src/click/core.py", line 2248, in get_default
    return super().get_default(ctx, call=call)
  File "/home/david/Projects/click/src/click/core.py", line 1808, in get_default
    return self.type_cast_value(ctx, value)
  File "/home/david/Projects/click/src/click/core.py", line 1840, in type_cast_value
    return tuple(self.type(x or (), self, ctx) for x in value or ())
  File "/home/david/Projects/click/src/click/core.py", line 1840, in <genexpr>
    return tuple(self.type(x or (), self, ctx) for x in value or ())
  File "/home/david/Projects/click/src/click/types.py", line 62, in __call__
    return self.convert(value, param, ctx)
  File "/home/david/Projects/click/src/click/types.py", line 749, in convert
    if len(value) != len(self.types):
TypeError: object of type 'int' has no len()

It fails in a different way if default=("a", "b").

Traceback (most recent call last):
  File "/home/david/Projects/click/example.py", line 5, in <module>
    value = opt.get_default(ctx)
  File "/home/david/Projects/click/src/click/core.py", line 2248, in get_default
    return super().get_default(ctx, call=call)
  File "/home/david/Projects/click/src/click/core.py", line 1808, in get_default
    return self.type_cast_value(ctx, value)
  File "/home/david/Projects/click/src/click/core.py", line 1840, in type_cast_value
    return tuple(self.type(x or (), self, ctx) for x in value or ())
  File "/home/david/Projects/click/src/click/core.py", line 1840, in <genexpr>
    return tuple(self.type(x or (), self, ctx) for x in value or ())
  File "/home/david/Projects/click/src/click/types.py", line 62, in __call__
    return self.convert(value, param, ctx)
  File "/home/david/Projects/click/src/click/types.py", line 750, in convert
    raise TypeError(
TypeError: It would appear that nargs is set to conflict with the composite type arity.

Environment

  • Python version: 3.8.5
  • Click version: 8.0.0.dev0
@davidism davidism added the f:parameters feature: input parameter types label Aug 17, 2020
@davidism davidism added this to the 8.0.0 milestone Aug 17, 2020
@davidism davidism self-assigned this Aug 17, 2020
@davidism davidism added the bug label Aug 17, 2020
@davidism
Copy link
Member Author

See #327, which introduced the issue. There is a lot of overlap between nargs, multiple, and types.Tuple that needs to be untangled.

@davidism
Copy link
Member Author

davidism commented Aug 17, 2020

Some discussion with @untitaker in #327. Concluded that multiple=True means "multiple of the same type", a homogeneous collection.

I think what needs to happen is detection should use the first item of a tuple as the type. If the default is the empty tuple, the type would be str just like if the default was None. In order to have multiple tuples, you would specify a list of tuples. default=(1, 2) would result in type=int. default=[(1, "a"), (2, "b")] would result in type=(int, str). Need to make sure this fits with nargs as well.

Detection doesn't validate that the default matches the type right now, so if someone did default=(1, "a") they would still get a type of int. A check that all the types in a tuple default are the same could be added to the __debug__ block.

@davidism
Copy link
Member Author

After reverting #327 and making some additional changes, the following works.

import click

opt = click.Option(["-c"], multiple=True, default=[(1, "a"), (2, "b")])
print(opt.type)
print(opt.type.types)
ctx = click.Context(click.Command("test"))
value = opt.get_default(ctx)
print(value)
<click.types.Tuple object at 0x7fabd06b4850>
[INT, STRING]
((1, 'a'), (2, 'b'))

This is a breaking change. The test that was added expected multiple=True, default=(1, "a") to mean --foo 2 b --foo 3 c. But it never tested using the default value, which was broken already. Now multiple=True, default=(1, "a") would be invalid because the default isn't homogeneous. multiple=True, default=(1, 2) would now mean --foo 2 --foo 3, not --foo 2 3 --foo 3 4. To get the existing test to pass again, you would pass multiple=True, default=[(1, "a")]. Given that the behavior was always incorrect, and the next version is a major release, I'm ok with this change.

@davidism
Copy link
Member Author

#1604 modified the end of full_process_value to check that nargs matches the length of the value. However, the check didn't look at multiple, so now that get_default returns a list of 2-tuples, nargs=2, multiple=True will fail if the default is a list of one 2-tuple. It also seems like type_cast_value should already be taking care of that check. Investigating why it doesn't led me to the following other issue.

#930 added a fix for an issue that prevented filling in the default when performing shell completion (resilient_parsing=True). For some reason, calling get_default is done in full_process_value, even though every other data source is checked in consume_value before full_process_value. The resilient_parsing check was only added to the default, but it seems like no source besides the command line should be used during completion.

What started as a simple fix in #1549 has turned into unraveling the entire processing pipeline. I'm starting to get behind #590 to rewrite the pipeline.

@davidism
Copy link
Member Author

davidism commented Aug 17, 2020

And finally, #472 and #767 are probably related as well, since even before the fix for this issue it wasn't possible to define an option with a tuple type without also defining a default, otherwise it would raise an error if the option wasn't given.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug f:parameters feature: input parameter types
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant