-
-
Notifications
You must be signed in to change notification settings - Fork 98
Add defaults for auto-populated selects #1804
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
base: master
Are you sure you want to change the base?
Conversation
I need to have a think about the restructure, but will most probably forget about it, so please ping me in the near future if I dont respond :) |
@@ -2086,6 +2207,10 @@ def add_select_menu( | |||
`MessageActionRowBuilder.add_channel_menu` and | |||
`MessageActionRowBuilder.add_text_menu`. | |||
|
|||
.. deprecated:: 2.0.0.dev123 |
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.
Just leaving this here as a note that these versions have to be bumped before this is merged
min_values: int = 0, | ||
max_values: int = 1, | ||
is_disabled: bool = False, | ||
default_values: undefined.UndefinedOr[ |
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.
This doesn't feel like the correct way to do this under how builders are usually implemented in this lib, wouldn't it be more natural to have these return a builder which you'd can call set_default_values then call the parent attribute to go back to the text input builder
Summary
Add
default_values
attribute for auto-populated selects. This did require some fairly significant restructuring to make type-safe, and honestly I'm still not quite happy with the result, so feedback is welcome.The select types have been completely seperated, so the new hierarchy of components looks something like this:
This PR also deprecates
ActionRowBuilder.add_select_menu()
as type-specific methods have been added for each select type. I feel like seperating the select types is better for type safety and potentially more future-proof if Discord decides add specific behaviours to a given select type down the line.Example
I tried an alternative interface where the snowflake's type could be inferred from the select's type, however this is not possible due to it being possible to mix default types in mentionable selects, so at the moment this is the best I could come up with.
The implementation tries to be type-safe to the extent that adding an invalid default type fails type-checking:
Checklist
nox
and all the pipelines have passed.Related issues
Closes #1776