-
-
Notifications
You must be signed in to change notification settings - Fork 132
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
feat(model, validate, cache)!: Add new select components #2051
Conversation
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.
Thanks for the PR! Overall this looks quite good. I have some comments on this on a high-level design.
Presently, we have only SelectMenu
"type", and that is what now maps to the StringSelectMenu
type. More or less the options in this type functions as a simple K-V choice. With this PR, we will now have three types: TypeSelectMenu
, which is in essense the "component base" with no additional fields; ChannelSelectMenu
, which is the base component but with a channel_types
field; and StringSelectMenu
, which is the base component with the options
field (and presently the only type with options).
TypeSelectMenu
covers the "mentionables", "roles", and "users" component types, which have no fields on top of the base component definition. If we think about how this might be extended in the future, I think it might not be unreasonable to conceptualize having some sort of user_types
field (with variants of users and bots), or some other sort of filter. We can also apply this to the roles selection, as "types" or "classes" of roles may be conceptualized (premium role filters? role tags? I don't know).
Were Rust an inheritance class-based language, we could simply call TypeSelectMenu
a SelectMenu
and have our ChannelSelectMenu
and StringSelectMenu
types superclass it. Alas, Rust is not, and extending types isn't so simple. Were it just the options
field that had a variable type we could use generics, but it is not. Select menus will probably continue to drift from how minimally different these three types are today.
This presents us with a question: do we create a SelectMenu*
type for every type of select menu and encapsulate them in a SelectMenu
enum? The benefit here could be that, in today's world, a UserSelectMenu
wouldn't share the fields that are unique to ChannelSelectMenu
and StringSelectMenu
. Typically, this feels better and is clearer for users, because it prevents one from even attempting to access the options of a RoleSelectMenu
, which conceptually doesn't apply to role select menus.
There is an alternative here: flatten it all into one type. This has been done before, and we could look to inspiration from those types on whether we should do the same here, Our most prominent example of this in history is the Channel
type: it was "flattened" from many types into one because there was a lot of overlap between them all, and because it closely resembles how Discord itself structures channels. There is no "audio channel" or "video channel" as such at an API level, there are only channels with those fields identifiable by the type
field. Channels with the "guild voice" type, once not able to "be a text channel", is now nearly equivalent to a "guild text" channel. Stage channels, once not able to be used with "video features", was showcased to soon support video during the December Discord Developers event.
Looking to Interaction
, it was flattened for a different reason: it was an enum with a unique type for each variant, such as an [ApplicationCommand
] type for the ApplicationCommand
variant, Ping
for the Ping
variant, ModalSubmitInteraction
for the ModalSubmit
variant, and so on. These shared the same base details about what an interaction is, and only contained some additional fields on top of it. Thus, we flattened it into one type, documented what applies when, and trusted users to know/check when something is present.
The case of SelectMenu
is perhaps not so different: the base fields of a select menu are largely shared, and there is only one field on top of them. channel_types
is somewhat obviously for specifying the types of channels and thus is known on a type name level to not be for, say, roles. options
is the only field remaining. If we leave options
unchanged then if any other select menu type supports options, the type of option (string select menu options) won't fit. Therefore, we should be prudent here and expect this possibility. There are a few ways we could go about this, such as making options
be an array of an enum with a variant being for string options or applying the same logic and making a base SelectMenuOption
struct type with value
being an enum. Or what if we made options
' type be an enum like this?
struct SelectMenuOption<T> {
...,
value: T,
}
enum SelectMenuOptions {
String(Vec<SelectMenuOption<String>>),
}
It may need touching up, but in my mind this last option blends the benefits of the other two. A select menu can only have one type of option, so its variant is asserted upfront rather than matched upon in the option itself (causing us to not have an enum of different option value types in the option). It also doesn't duplicate types, leaving the type of value in the signature itself.
This is my first impression on where we should go with this, and what routes we've gone in similar situations. Let me know if you have any questions or thoughts, since I'm definitely open to debate on this.
Debug, | ||
Deserialize, | ||
Eq, | ||
// Hash, |
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 a pr comment on this (todo?) for visibility
); | ||
assert_impl_all!( | ||
MessageComponentInteractionData: Clone, | ||
Debug, | ||
Deserialize<'static>, | ||
Eq, | ||
Hash, | ||
// Hash, |
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 a pr comment on this (todo?) for visibility
Would it be reasonable to use generics and add markers for the select menu, something like the ids do? |
Do you mean a method for extracting the target |
Adds support for new select components in twilight-model and the twilight-validation crates. See Discord changelog.
I believe that I have included all the necessary changes relating to them but it is possible I missed something.
Not sure if this is how you guys want them implemented. Just let me know or someone else can take it over if you'd like 😄
Breaking changes
We don't need to rename these and I can revert them back but I do think it's a good change to make since it clarifies exactly what can be done with them. Discord documentation also refers to them as such.
Additions
I added a TypeSelectMenu since 3 (user, mentionable, & role) of the 4 new selects use the same data structure. Not sure if this should be named something else.
Channel selects got their own type, ChannelSelectMenu, which lets you filter which channels are shown by setting channel_types.
Tests
Added and updated a few tests but likely need to add some more. Just not sure what you want specifically tested. I did manually test with one of my bots & they do work.