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

Support data types other than string for click.Choice values #591

Closed
AldoLonghi opened this issue Jun 2, 2016 · 7 comments
Closed

Support data types other than string for click.Choice values #591

AldoLonghi opened this issue Jun 2, 2016 · 7 comments
Labels
f:parameters feature: input parameter types

Comments

@AldoLonghi
Copy link

Several times I have used click.Choice where I want my set of values to be integers. However this class requires that the values be strings, and the resulting value is also a string (requiring conversion in my code before and after). Is it not possible to allow other types? Certainly it would be reasonable to raise an exception if the set of choices contained a variety of types, but as long as they were consistently one type this seems a useful improvement. I'd be happy to work on such a feature, but wanted to check if this seems reasonable before I expend the effort. Thanks! (I LOVE the library, by the way!)

@ssbarnea
Copy link

Click should at least convert to strings all elements from the iterator received, which is something very easy to do that will sort most of this problem.

ssbarnea added a commit to ssbarnea/click that referenced this issue Sep 15, 2019
Fixes problem where the iterator may return different objects, making
choice fail when trying to use string specific methods.

Example:

    return '[%s]' % '|'.join(self.choices)
TypeError: sequence item 0: expected str instance, XXX found

Fixes: pallets#591
ssbarnea added a commit to ssbarnea/molecule that referenced this issue Sep 15, 2019
Avoids pallets/click#591 by converting choices
to lists of strings.

Signed-off-by: Sorin Sbarnea <[email protected]>
@altendky
Copy link
Contributor

@ssbarnea, I don't understand how click converting integers to strings helps us get integers passed to our decorated functions.

@ssbarnea
Copy link

@altendky AFAIK, all my functions do assume that they will receive strings because CLI works only with strings anyway. You cannot pass real integers to a program, they are all in the end just strings.

@altendky
Copy link
Contributor

@ssbarnea, in the command line yes, but the point is that it is nice to have an abstraction between that extremely limited environment and Python. Consider that even click itself already has simple types like int and even fancy ones like opened files that are written atomically.

@ssbarnea
Copy link

@altendky You are right. This means that my patch would not fix this issue but is is still valid, fixing the display of non strings objects, right?

ssbarnea added a commit to ssbarnea/click that referenced this issue Sep 15, 2019
Fixes problem where the iterator may return different objects, making
choice fail when trying to use string specific methods.

Example:

    return '[%s]' % '|'.join(self.choices)
TypeError: sequence item 0: expected str instance, XXX found

Fixes: pallets#591
ssbarnea added a commit to ansible/molecule that referenced this issue Sep 15, 2019
Avoids pallets/click#591 by converting choices
to lists of strings.

Signed-off-by: Sorin Sbarnea <[email protected]>
@altendky
Copy link
Contributor

@ssbarnea, that may be a separate thing that some people want but you'll have to have your code be expecting the string form anyways at which point it seems you may as well just have passed in strings to begin with. But I'm admittedly on the go and haven't thought about this a lot right now.

It might be more reasonable to have a thing (maybe enhanced click.Choice, maybe something else) that takes a mapping from strings to whatever. Given values = [9600, 19200, 57600] you could pass {str(value): value for value in values}. Keep in mind that in other cases you may easily need something other than str() to create the strings.

Anyways, maybe what this ticket needs is example uses to get any idea of what cases need to be covered. I feel like there may also be some simple patterns that might relate to this that could be implemented with existing options but they aren't coming to mind presently.

Relates to #605 though it is specifically for enums.

@kx-chen kx-chen added the f:parameters feature: input parameter types label Jul 14, 2020
@davidism
Copy link
Member

Regarding coercing other types to str, I closed the PR in favor of adding annotations instead of runtime checks. I think I'd like to keep Choice as-is right now. If you need to map to other types, you can either subclass and override Choice.convert, or add a callback to the parameter.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
f:parameters feature: input parameter types
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants