-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Closed
+185
−0
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
from __future__ import annotations | ||
|
||
import collections.abc as cabc | ||
import enum | ||
import os | ||
import stat | ||
import sys | ||
|
@@ -336,6 +337,23 @@ def shell_complete( | |
return [CompletionItem(c) for c in matched] | ||
|
||
|
||
class EnumChoice(Choice): | ||
def __init__(self, enum_type: type[enum.Enum], case_sensitive: bool = True): | ||
super().__init__( | ||
choices=[element.name for element in enum_type], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
case_sensitive=case_sensitive, | ||
) | ||
self.enum_type = enum_type | ||
|
||
def convert( | ||
self, value: t.Any, param: Parameter | None, ctx: Context | None | ||
) -> t.Any: | ||
value = super().convert(value=value, param=param, ctx=ctx) | ||
if value is None: | ||
return None | ||
return self.enum_type[value] | ||
|
||
|
||
class DateTime(ParamType): | ||
"""The DateTime type converts date strings into `datetime` objects. | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 theenum_type.__members__
mapping to get all names including aliases:Or, and this may be even better, map aliases to canonical names in
convert()
, before passing on the value tosuper().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 inconvert()
:There was a problem hiding this comment.
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:
I guess this would either require reimplementing the
case_sensitive
handling insideEnumChoice.convert
, or actually having all possible values, including aliases, inEnumChoice.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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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:
But the following would also work:
There's also
enum_type.__members__.keys()
, but that's not a sequence, it's just iterable.There was a problem hiding this comment.
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:
This does start to get somewhat repetitive with the code in
Choice
, and I did ignore the optionalctxt.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 thecase_sensitive
flag andctx
.There was a problem hiding this comment.
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 breakshell_complete
I believe.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.