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

Add EnumChoice parameter type #2210

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

saroad2
Copy link
Collaborator

@saroad2 saroad2 commented Mar 10, 2022

Added a new ability to be able to create a choice parameter type from enum class.

How does it differ from Choice? Enums are the way of python to allow only a finite number of values to be accepted in a given scenario. It seems logical to me that click should offer a Choice-like parameter type that gets its values from an enum and returns an enum.

Now, using click.EnumChoice, one can create a Choice-like parameter that returns an enum instead of a string.
Moreover, once you add a new value to your enum class, it will automatically update as a valid choice for the paramater type.

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@saroad2
Copy link
Collaborator Author

saroad2 commented Mar 25, 2022

Hey @davidism .

I would love to hear your feedback on this PR.

@saroad2
Copy link
Collaborator Author

saroad2 commented Apr 25, 2022

Does this PR need any changes?
Is it click 8.2.0 material? 9.0.0?

I would love to know so I could work on it if any changes are necessary.
I think this feature could really help the users of click.

@davidism
Copy link
Member

The PR itself seems fine, but I'm not sure I want to merge it yet. You'll know when I make a decision because I'll update it one way or another.

@dzcode dzcode mentioned this pull request May 2, 2022
6 tasks
@MicaelJarniac
Copy link

Related to #605.

@sscherfke
Copy link
Contributor

Why wouldn't you want to merge that? (Serious question)

What's in favor:

  • Enums have been in the stdlib for a very long time now and lead to cleaner and more robust code for exactly these kind of "choice problems".
  • This PR looks quite clean. It adds little new code and has many tests.
  • There seems to be a high demand for this feature since nearly 6 years. People have been updating the example in the Issue if it stopped working after a click update.

Looking forward to hearing your objections, @davidism :-)

@sscherfke
Copy link
Contributor

@saroad2 If David would consider merging this PR, you should add tests using EnumChoice with a default value (because you cannot use MyEnum.option have to use MyEnum.option.name. And I assume you will also add documentation than? :-)

@saroad2
Copy link
Collaborator Author

saroad2 commented May 9, 2022

Hey @sscherfke , once David will approve that he intend on merging this PR (in the near or far future) I will add the relevant documentation and add any test that might be relevant.

@AndreasBackx
Copy link
Collaborator

Hey @davidism, have you been able to make a decision? 😃 I think @saroad2 would love to move forward with the PR and get this available for all Click users.

@saroad2
Copy link
Collaborator Author

saroad2 commented Nov 9, 2022

This PR still waits for @davidism approval.

I know there are issues about testing and API in this PR, but I can't work on it until David do an initial review.

Would love to see it getting merged.

@RonnyPfannschmidt
Copy link
Contributor

@davidism i'd love to see this land, after all i just shipped a broken release of a cli due to a mistake with removing a choice and missing a purely stringly usage on a default all the tests would pass in correctly

had i been able to use enum i wouldn't have missed that

class EnumChoice(Choice):
def __init__(self, enum_type: t.Type[enum.Enum], case_sensitive: bool = True):
super().__init__(
choices=[element.name for element in enum_type],
Copy link
Contributor

@mjpieters mjpieters May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider including aliases in this; either as an explicit option (include_aliases: bool = False) or by default. Loop over the enum_type.__members__ mapping to get all names including aliases:

Suggested change
choices=[element.name for element in enum_type],
choices=[name for name in enum_type.__members__],

Or, and this may be even better, map aliases to canonical names in convert(), before passing on the value to super().convert(), so that the choices listed in help documentation don't include the aliases.

In that case, store the aliases here in __init__ and reference those in convert():

        # ...
        self.enum_type = enum_type
        self.aliases = {
            alias: enum_type[alias].name
            for alias in set(enum_type.__members__).difference(self.choices)
        }

    def convert(
        self, value: t.Any, param: t.Optional["Parameter"], ctx: t.Optional["Context"]
    ) -> t.Any:
        value = self.aliases.get(value, value)
        value = super().convert(value=value, param=param, ctx=ctx)
        # ...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't that break things like the case_sensitive flag?
Like, if you have something like:

import enum

class MyEnum(enum.Enum):
    Foo = 0
    Bar = 1
    Baz = 1  # Alias for `Bar`

# Inside `EnumChoice`:
choices = ["Foo", "Bar"]
aliases = {"Baz": "Bar"}

# And in the `EnumChoice.convert` method:
value = "baz"  # User input
value = aliases.get(value, value)  # "baz" not in `aliases`
# `value` is still "Baz"
# When passed to `super().convert(...)`, it won't know what to do with it

I guess this would either require reimplementing the case_sensitive handling inside EnumChoice.convert, or actually having all possible values, including aliases, in EnumChoice.choices.

In my opinion, reimplementing the flag would be sketchy, so I'd probably suggest having all aliases in the choices.

One thing that might be possible, but I have no idea if it actually is, would be to then modify how the argument documentation is generated, to either omit the aliases or to document them together with the primary choice.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not having aliases in EnumChoice.choices would also break shell completion for the aliases. This may or may not be undesirable.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing, on your suggested change you use:

choices=[name for name in enum_type.__members__],

But the following would also work:

choices=list(enum_type.__members__)

There's also enum_type.__members__.keys(), but that's not a sequence, it's just iterable.

Copy link
Contributor

@mjpieters mjpieters May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh, yes, list(enum_type.__members__) is the obvious choice. I also don't use the case-sensitive option so I indeed missed that.

In that case, case-fold the aliases in the mapping:

        # ...
        self.enum_type = enum_type
        self.aliases = {
            alias if case_sensitive else alias.casefold(): enum_type[alias].name
            for alias in set(enum_type.__members__).difference(self.choices)
        }
        
    def convert(
        self, value: t.Any, param: t.Optional["Parameter"], ctx: t.Optional["Context"]
    ) -> t.Any:
        value = self.aliases.get(value if self.case_sensitive else value.casefold(), value)
        value = super().convert(value=value, param=param, ctx=ctx)
        # ...

This does start to get somewhat repetitive with the code in Choice, and I did ignore the optional ctxt.token_normalize_func() function. Perhaps the normalisation (case folding, ctx.token_normalize_func() support) could be factored out into a supporting function that can then be reused here. Something that generates a transformation function based on the case_sensitive flag and ctx.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not having all values in choices will also break shell_complete I believe.

Copy link
Contributor

@mjpieters mjpieters May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not having all values in choices will also break shell_complete I believe.

No, it won't break completion. You won't be able to use the aliases as completion words but that's not necessarily the point of them, but nothing breaks when you try to complete against a partial alias. You just don't get a completed word at that point.

@saroad2
Copy link
Collaborator Author

saroad2 commented May 2, 2023

Hey @mjpieters, thanks for the review.

Are you from the Pallets team? Until someone from them take the time and address this PR, I'm not going to spend time fixing issues here. This PR is sitting here for over a year. I thought it would get merged by now.

@RonnyPfannschmidt
Copy link
Contributor

i'll try to bring it up again on the discord, i'd love to see it land but i dont call the shot

@davidism
Copy link
Member

davidism commented May 2, 2023

Click or Jinja is my next focus, as soon as the new Flask and Werkzeug releases settle down. I had been focused almost entirely on Flask and Werkzeug for the last year.

@mjpieters
Copy link
Contributor

Hey @mjpieters, thanks for the review.

Are you from the Pallets team? Until someone from them take the time and address this PR, I'm not going to spend time fixing issues here. This PR is sitting here for over a year. I thought it would get merged by now.

I'm not a member of the pallets team, no, sorry.

I have been using this implementation in production code, and found aliases to be a great way to improve a CLI where the spelling of some of the choices could cause confusion. Aliases solved those issues very, very nicely, without cluttering up the UI further.

So, I'd really love to see this become part of core click package, but with some form of aliases support included.

@saroad2
Copy link
Collaborator Author

saroad2 commented May 2, 2023

Click or Jinja is my next focus, as soon as the new Flask and Werkzeug releases settle down. I had been focused almost entirely on Flask and Werkzeug for the last year.

Thanks @davidism , I appriciate you. I hope to see it merged soon.

Click is used by a lot of users, and personally I find myself use it in most of my projects. Maybe it's time to consider adding more maintainers to this project for more routinely checks of this project. If you open up a call, I would happily suggest myself. I'm sure many others would too.

As for this perticular PR, once you review it, I would gladly start fixing issues here.

@davidism
Copy link
Member

davidism commented May 2, 2023

I'm definitely open to adding more long-term contributors, especially around reviewing existing PRs and triaging issues. Ping me in https://discord.gg/pallets #click channel about getting set up.

@davidism davidism added this to the 8.2.0 milestone Jun 30, 2023
@aldanor
Copy link

aldanor commented Jul 9, 2023

@saroad2 (+@mjpieters)

It might also be nice to allow constructing enums from member values and not keys, i.e. by using Enum(x) as opposed to Enum[x]. Note that Enum(x) would also respond to the built-in _missing_() functionality allowing you to concoct whatever you want in terms of aliases etc whereas Enum[x] would not.

Using __getitem__ on the enum type itself is pretty restrictive since in order to override it you need to mess with metaclasses whereas __init__ allows for the _missing_() workaround.

E.g. in cases like

class MyEnum(Enum):
    SNAKE_CASE_NAME = 'first'
    TOO_MANY_UNDERSCORES = 'second'

    @classmethod
    def _missing_(cls, obj):
        if obj == '1':
            return cls('first')
        elif obj == '2':
            return cls('second')
        return None

you would probably want it to respond to 1, 2, first and second rather than SNAKE_CASE_NAME and TOO_MANY_UNDERSCORES in the CLI.

This might also partially solve the case-sensitivity question (in fact, that's the example for _missing_() in the official docs).

@saroad2
Copy link
Collaborator Author

saroad2 commented Aug 21, 2023

Hey everyone!

I'm glad to see so many changes to Click since I last logged in to this project!
I'd happily work on this PR and prepare it for the merge.

But first, I still need @davidism, as the primary maintainer of this project, to review it beforehand since this is a new feature.

Once that happens, I'll work on addressing everyone's comments.

Meanwhile, I merged the main branch into the PR to make it up-to-date.

class EnumChoice(Choice):
def __init__(self, enum_type: t.Type[enum.Enum], case_sensitive: bool = True):
super().__init__(
choices=[element.name for element in enum_type],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Should there be the ability to use the enum values as choices, enabled through a parameter?
  • Should there be the ability to use the enum member itself as the default, which I believe would require adding the enum members themselves to choices, not just their string name? e.g. default=SomeEnum.member

@maikeriva
Copy link

Hi! Any progress on this? Some way I could help?

@cjw296
Copy link

cjw296 commented Sep 6, 2024

Sad to see that after 8 yrs since the initial issue was logged there's still no native support for enums and string enums as click choices :-(

What can we do to change this?

@AndreasBackx AndreasBackx mentioned this pull request Oct 20, 2024
39 tasks
@AndreasBackx
Copy link
Collaborator

I was looking into what was needed to get this into 8.2.0. I think we should do the following:

Include @mjpieters's suggestion to use __members__ to make aliases possible. However, instead of working around the implementation of Choice, change the implementation so aliases don't need special handling. We can do this by not relying on the Enum to str conversion and storing the concrete Enum value. I've done this as part of #2796.

This by itself should remove the need for an EnumChoice wrapper and allow you to pass an Enum to Choice. See #2796 for more info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.