feat(twilight-model)!: Implement additional select menu types #2219

archer-321 · 2023-06-12T18:56:17Z

This merge request implements Discord's new select menu types (user, role, mentionable, and channel) using the fully-flattened approach outlined in this RFC.

General idea

The general idea is to implement all variant-specific fields as Options inside the SelectMenu struct. While this might require runtime validation, it's more in line with the rest of the Twilight ecosystem. Moreover, it doesn't duplicate fields that an approach with separate structs would duplicate.

Notably, this differs from the RFC mentioned above. This RFC was indirectly denied with this review.

Breaking changes

This PR introduces multiple breaking changes:

ComponentType's SelectMenu variant was renamed, and the enum was extended (breaking as it's not #[non_exhaustive]).
SelectMenu's options field was wrapped in an Option.
New channel_types and kind fields were introduced in SelectMenu.
twilight_model::application::interaction::application_command::{CommandInteractionDataResolved, InteractionChannel, InteractionMember} were moved into twilight_model::application::interaction.
CommandInteractionDataResolved was renamed to InteractionDataResolved.
MessageComponentInteractionData no longer implements Eq + Hash.
A new resolved field was introduced in MessageComponent.
InteractionData::MessageComponent's single field was Box-ed.

If I missed any breaking changes, please point them out.

Related work

This PR closes Support all Select Menu types #2047.
As mentioned above, this implements an idea outlined in RFC: Implement different select menus with a `SelectMenuType` enum #2217.
The same feature was previously implemented in feat(model, validate, cache)!: Add new select components #2051, but that PR was seemingly abandoned.
This feature is defined by Discord's documentation
I created a throwaway bot to test this PR here: https://github.com/archer-321/twilight-select-menus.

Tests

This PR only updates the existing tests. If you'd like me to add other tests to verify the behaviour introduced by this patch, please feel free to request changes.

Signed commits

I'm unsure whether this is possible with Twilight's PR flow, but please ask me to squash any commits or edit commit messages if necessary. I prefer all commits attributed to me to be signed with my GPG key 🤓

Labels

Based on the current state, I believe you should assign the d-api and d-breaking labels.

This patch implements the additional select menu types `user`, `role`, `mentionable`, and `channel`. Moreover, it moves the type-specific data for the existing text select menu implementation into a separate enum variant. The new types are implemented by "semi-flattening" the `SelectMenu` struct: fields common to all select menu types are implemented in the struct itself, and type-specific fields like the available options or channel types are moved into a `SelectMenuData` enum. This enum is also used to select a select menu's type. This approach de-duplicates the common fields while preventing users from accessing fields irrelevant to the current select menu type. Finally, this commit updates the documentation and existing tests to reflect these changes. The tests, however, might require additional attention in case new behaviour introduced by this commit should be tested as well.

baptiste0928

Thanks for this PR, it looks good to me overall!

It seems that you should also add a resolved field to MessageComponentInteractionData (Discord documentation). This has been implemented in #2051 if you need a reference.

Moreover, those instances are Box-ed inside the enum to avoid unnecessarily increasing the enum's memory footprint if Discord adds new fields in the future.

This need to be discussed, but I personally think it just adds unnecessary complexity. Discord is unlikely to add fields that make the enum very large - and we can't anticipate everything anyway.

twilight-model/src/channel/message/component/mod.rs

This patch makes `ComponentVisitor::visit_map` use `unreachable!` instead of `panic!` in case a select menu type was not implemented while being listed in a previous match statement. Moreover, this commit sneaks in a minor formatting fix inside `Components` doc-comment example.

This commit implements the `resolved` field for `MessageComponentInteraction`. This field holds resolved users, roles, channels, or attachments for select menu interactions. Unfortunately, the new field makes it impossible to implement `Eq` and `Hash` for `MessageComponentInteractionData`.

This commit adds a simple Discord bot that creates and responds to select menus using Twilight. The main purpose of this bot is to demonstrate the API introduced by GH-PR twilight-rs/twilight#2219. Specifically, the code in this repository is a "quick and dirty" test, and its code is not scalable, idiomatic, or in any way recommendable outside testing the API.

archer-321 · 2023-06-13T17:45:26Z

It seems that you should also add a resolved field to MessageComponentInteractionData (Discord documentation). This has been implemented in #2051 if you need a reference.

Indeed! I'm not sure how I missed this. I took the opportunity to create a small throwaway bot here: https://github.com/archer-321/twilight-select-menus.

It responds to simple legacy text commands and sends a message with all select menu types.

Screenshot

This need to be discussed, but I personally think it just adds unnecessary complexity. Discord is unlikely to add fields that make the enum very large - and we can't anticipate everything anyway.

I agree that it adds extra complexity and might be unnecessary. Still, I'm unsure whether the decreased complexity of removing it outweighs the benefit of having each variant be the size of a pointer. Let's wait for other reviews c:

This commit fixes the tests testing the (de-)serialization of `InteractionDataResolved`. Previously, the tests were failing, as the type was renamed from `CommandInteractionDataResolved`. Moreover, this commit fixes the message component interaction tests by adding the missing `resolved` field to the expected fields in `message_component_interaction_data`.

baptiste0928 · 2023-06-13T18:17:50Z

Indeed! I'm not sure how I missed this. I took the opportunity to create a small throwaway bot here: https://github.com/archer-321/twilight-select-menus.

It's not documented in Message Component Data Structure, I only saw it because the previous PR had implemented it 😅

Thanks for the throwaway bot, it's really useful for reviewing :)

the benefit of having each variant be the size of a pointer.

The enum is 32 bytes without Box, 16 bytes with. There is no benefit, it's even worse because you have to make a heap allocation.

This commit unboxes `SelectMenuData`'s variants. They were boxed in a previous patch to prevent future fields from unnecessarily increasing the enum's memory footprint, but considering that, with the current structures, new fields are a breaking change already, it's OK to leave the variants unboxed for now and re-box as necessary. For the current code, this saves a heap allocation while only marginally increasing the enum's memory footprint. This change is only breaking in the context of PR twilight-rs#2219. When added to the commits in this PR, it doesn't add any breaking changes to the PR's list of breaking changes.

archer-321 · 2023-06-14T14:19:02Z

The enum is 32 bytes without Box, 16 bytes with. There is no benefit, it's even worse because you have to make a heap allocation.

I see. Considering that additional fields would be a breaking change anyway, I agree that boxing the enum variants is unnecessary.

As discussed on Discord, I marked the PR as ready. After all, approving the implementation implicitly approves the general design (and thus the RFC).

HTGAzureX1212

LGTM.

Gelbpunkt

I fundamentally disagree with the design choices in this PR, for several reasons. By "the design choices", I mean not flattening the struct.

If we were to make the change from flattened structs back to enums, that should be done for all model structs and not for a single struct. That is inconsistent and, in my view, a code smell.
We have in the past switched from enums to flattened structs for things like channel due to "[flattening] will prevent breaking changes and API oddities in the future" (a0ec109). Flip-flopping back and forth is not really desirable for maintainers or users.
Field access gets really awkward with inner enums for fields present conditionally. some_select_menu.options as Option<Vec<SelectMenuOption>> makes accessing the field much easier than if let matching on parts of a struct which are contained in an enum.
Flattened structs model the Discord API closer and will be less likely to cause breaking changes (this kind of repeats the argument from 2.). If options is ever going to be present for other types than 3, it won't require breaking public API changes. This also models the Discord documentation a bit closer (link).

TLDR: I don't see the added value at all. I'd be happy to accept this if we would just include the struct fields flattened as options, or if we revisit the entire model crate and make serious future commitments instead of changing model philosophy every few releases.

HTGAzureX1212 · 2023-06-14T23:33:38Z

I fundamentally disagree with the design choices in this PR, for several reasons. By "the design choices", I mean not flattening the struct.

If we were to make the change from flattened structs back to enums, that should be done for all model structs and not for a single struct. That is inconsistent and, in my view, a code smell.

We have in the past switched from enums to flattened structs for things like channel due to "[flattening] will prevent breaking changes and API oddities in the future" (a0ec109). Flip-flopping back and forth is not really desirable for maintainers or users.

Field access gets really awkward with inner enums for fields present conditionally. some_select_menu.options as Option<Vec<SelectMenuOption>> makes accessing the field much easier than if let matching on parts of a struct which are contained in an enum.

Flattened structs model the Discord API closer and will be less likely to cause breaking changes (this kind of repeats the argument from 2.). If options is ever going to be present for other types than 3, it won't require breaking public API changes. This also models the Discord documentation a bit closer (link).

TLDR: I don't see the added value at all. I'd be happy to accept this if we would just include the struct fields flattened as options, or if we revisit the entire model crate and make serious future commitments instead of changing model philosophy every few releases.

To be fair. I would agree if the models involved in the enum have more fields in common - but this does not seem to be the case. I believe that if there are many common fields for different types in the enum, then sure flattening would be better (reduce duplication and also memory footprint). But in this case I do not see much point flattening albeit conflicting with the current design choices - as far as I know there are not much fields that are in common between different select menu types (perhaps only 1 field?).

~~Not really sure to be honest, but this indeed can spark further discussion.~~

Nevermind, I read the documentation for one more time and it seems like flattening is better.

Gelbpunkt · 2023-06-14T23:36:51Z

There are 6 common fields and 2 conditional ones, you can check it here.

ImDarkDiamond · 2023-06-14T23:40:26Z

I'm also for flattening. After using the flattened channel struct for a while now I find it much nicer than matching on an enum

HTGAzureX1212

As per comments made by @Gelbpunkt, flattening the structures will align better with the Discord API model as well as our design philosophy for model structures.

archer-321 · 2023-06-15T01:06:24Z

TLDR: I don't see the added value at all

I'm a huge fan of static enforcement for types, and I aim to implement it through my libraries wherever possible - even if it comes at the cost of increasing the required boilerplate. E.g. it's less convenient to match an Option than being able to dereference a pointer, but Options prevent null pointer dereferencing at compile time.

That said, it's way less dramatic with the entirely flattened approach. Instead of crashing the program, the struct would allow users to specify invalid select menus (e.g. a channel select menu that tries to set the options field).
Considering that Discord would return an error if a developer tries to do this, I can see how the increased convenience can outweigh the added value of static enforcement.

Of course, I can flatten the struct in this PR, but I have a couple of questions regarding the desired implementation:

How would users specify which select menu type they want to implement? User, role, and mentionable select menus don't add any type-specific fields, so there needs to be an additional field (e.g. kind) to select the type. However, having another kind type would duplicate the kind already present in the component (de-)serialization. Considering that Discord assigns different type values to the different select menu types, this approach doesn't follow the upstream API either.
As outlined in feat(model, validate, cache)!: Add new select components #2051 (review), "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". Should I follow the approach explained in this comment to move the implementation of the current options into a separate enum?

laralove143 · 2023-06-15T09:57:55Z

i think this discussion should be moved to #2217 and that discussion should be renamed to reflect an overall consistent change in the model crate and then we should merge or request changes in the pr before merging it based on the result

we should first decide how we handle rfcs though

HTGAzureX1212 · 2023-06-16T05:10:03Z

i think this discussion should be moved to #2217 and that discussion should be renamed to reflect an overall consistent change in the model crate and then we should merge or request changes in the pr before merging it based on the result

we should first decide how we handle rfcs though

Do we want a similar procedure with that of Rust’s?

laralove143 · 2023-06-16T12:15:43Z

i think this discussion should be moved to #2217 and that discussion should be renamed to reflect an overall consistent change in the model crate and then we should merge or request changes in the pr before merging it based on the result

we should first decide how we handle rfcs though

Do we want a similar procedure with that of Rust’s?

partially yeah i agree but i think we can keep it simpler since i scope is smaller i think the flow should be like

rfc -> contributing.md (if accepted) -> adapting twilight for rfc changes

archer-321 · 2023-06-20T17:57:25Z

Could we progress with the RFC or this PR? If the static enforcement introduced by this change isn't an option because it would differ from the rest of the library, I'd be OK with implementing the entirely flattened approach. However, I'm still unsure about the two questions listed in #2219 (comment) 😟

In my opinion, an RFC wouldn't be needed when going for the flattened approach that aligns with other types in Twilight. Instead, a more general RFC should discuss the benefits and drawbacks of changing structs to a partially flattened implementation throughout the library, in this case.

HTGAzureX1212 · 2023-06-21T01:11:27Z

Could we progress with the RFC or this PR? If the static enforcement introduced by this change isn't an option because it would differ from the rest of the library, I'd be OK with implementing the entirely flattened approach. However, I'm still unsure about the two questions listed in #2219 (comment) 😟

In my opinion, an RFC wouldn't be needed when going for the flattened approach that aligns with other types in Twilight. Instead, a more general RFC should discuss the benefits and drawbacks of changing structs to a partially flattened implementation throughout the library, in this case.

Yeah, it would be nice if you could progress with implementing the flattened approach.

archer-321 · 2023-06-21T02:05:02Z

I'm sorry if my last message sounded impatient or demanding; that wasn't my intention.

I agree; for now, an approach that's more in line with the rest of the library is probably best. However, I still have the two questions I linked to in my last message.

Specifically, I still have the two questions quoted below ⬇️

How would users specify which select menu type they want to implement? User, role, and mentionable select menus don't add any type-specific fields, so there needs to be an additional field (e.g. kind) to select the type. However, having another kind type would duplicate the kind already present in the component (de-)serialization. Considering that Discord assigns different type values to the different select menu types, this approach doesn't follow the upstream API either.

As outlined in
feat(model, validate, cache)!: Add new select components #2051 (review), "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". Should I follow the approach explained in this comment to move the implementation of the current options into a separate enum?

I can't think of a satisfying implementation that addresses both problems (preventing a duplicate kind field and future-proofing the API if Discord adds more select menu options).

Before I spend more hours creating another patch that fundamentally doesn't align with the crate's vision, I'd like some input before I start changing the API again. I hope you understand 🙂

baptiste0928 · 2023-06-23T19:23:18Z

How would users specify which select menu type they want to implement?

A kind field seems like a good solution to me. There is no duplication in the public API (except for the kind() method), only in the implementation, so it's not confusing imo.

Should I follow the approach explained in this comment to move the implementation of the current options into a separate enum?

Personally, I think it's best to focus on a simple implementation. We can't anticipate all the changes Discord will make, so it's not worth making our API more complicated for changes we don't know will happen. I'd be curious to know what others think about this.

This commit removes the SelectMenuData type and embeds all variant-specific fields in the base struct. Moreover, it adds documentation indicating which fields are required variant-specific fields. This commit also updates the component validation module in `twilight-validate` by introducing a new error type for text select menus that don't have the necessary `options` field.

This commit updates the bot to reflect the most recent changes to twilight-rs/twilight#2219 introduced in c774adc0. Specifically, the `SelectMenu` struct was flattened to include all variant-specific fields. Moreover, a new `kind` field is used instead of `data` to determine the select menu variant.

archer-321 · 2023-06-24T18:32:25Z

I implemented the entirely flattened approach requested above and updated the test bot. If I missed something, please let me know.

A kind field seems like a good solution to me.

Done ✔️

Personally, I think it's best to focus on a simple implementation. We can't anticipate all the changes Discord will make, so it's not worth making our API more complicated for changes we don't know will happen. I'd be curious to know what others think about this.

Considering that the API isn't stable across releases at this point in Twilight's development, this would likely be overkill. For now, I implemented the fields directly.

Gelbpunkt

Overall code LGTM, I have some nits on documentation but I'm not a native speaker and there are other people who might disagree so I'd be fine with merging this as-is, consider my comments as what went through my mind when reading this

twilight-model/src/channel/message/component/select_menu.rs

Gelbpunkt · 2023-06-25T13:48:14Z

twilight-model/src/channel/message/component/select_menu.rs

+pub enum SelectMenuType {
+    /// Select menus with a text-based `options` list.
+    ///
+    /// Select menus of this `kind` *must* set the `options` field to specify the options users


I don't think we should be writing API documentation for Discord in all places imaginable, one should be enough on the options field

I mention this in the enum documentation, as users may only read its documentation to see which select menu types are available.
Now that the options field is an Option<_>, I think the duplicated documentation is worth it if it saves some people from debugging a Discord API error.
However, I'm open to talking about this instance.

any other opinions on this or do we leave it as is?

I agree upon that this duplicated documentation is worth it; in terms of helping users understand the requirements on fields for specific select menu types.

As such, I believe that we can keep this as is.

twilight-model/src/channel/message/component/select_menu.rs

This commit streamlines the language in the `SelectMenu` and `SelectMenuType` documentation. In particular, it removes duplicated sections outlining required and optional fields. Moreover, it makes language describing which menu types use a specific option more in line with the rest of the library.

vilgotf · 2023-07-01T05:50:25Z

This PR has three approvals but is still labled as unapproved. What's holding this up?

Gelbpunkt · 2023-07-01T13:17:59Z

This PR has three approvals but is still labled as unapproved. What's holding this up?

Nothing, I forgot to remove the label after the PR switched to flattened models

HTGAzureX1212 · 2023-07-01T13:19:51Z

This PR has three approvals but is still labled as unapproved. What's holding this up?

Nothing, I forgot to remove the label after the PR switched to flattened models

Then I presume that this pull request can be merged.

vilgotf · 2023-07-01T17:05:46Z

Amazing work, thank you so much! 🎉

github-actions bot added c-model Affects the model crate c-validate Affects the validate crate m-breaking change Breaks the public API. t-feature Addition of a new feature labels Jun 12, 2023

Gelbpunkt added d-api Change related to Discord's API. w-unapproved Proposal for change has *not* been approved by @twilight-rs/core. d-breaking Code breaks the API because of Discord labels Jun 12, 2023

baptiste0928 self-requested a review June 12, 2023 20:09

baptiste0928 reviewed Jun 13, 2023

View reviewed changes

twilight-model/src/channel/message/component/mod.rs Outdated Show resolved Hide resolved

archer-321 added 2 commits June 13, 2023 14:55

github-actions bot added c-cache Affects the cache crate c-standby Affects the standby crate labels Jun 13, 2023

archer-321 requested a review from baptiste0928 June 13, 2023 17:45

baptiste0928 approved these changes Jun 13, 2023

View reviewed changes

archer-321 marked this pull request as ready for review June 14, 2023 14:15

Gelbpunkt added the w-do-not-merge PR is blocked or deferred label Jun 14, 2023

Gelbpunkt self-requested a review June 14, 2023 14:26

HTGAzureX1212 self-requested a review June 14, 2023 14:38

HTGAzureX1212 approved these changes Jun 14, 2023

View reviewed changes

Gelbpunkt requested changes Jun 14, 2023

View reviewed changes

HTGAzureX1212 self-requested a review June 14, 2023 23:40

HTGAzureX1212 reviewed Jun 14, 2023

View reviewed changes

archer-321 requested review from Gelbpunkt and baptiste0928 June 24, 2023 18:32

baptiste0928 approved these changes Jun 25, 2023

View reviewed changes

Gelbpunkt approved these changes Jun 25, 2023

View reviewed changes

Gelbpunkt removed w-do-not-merge PR is blocked or deferred w-unapproved Proposal for change has *not* been approved by @twilight-rs/core. labels Jul 1, 2023

vilgotf merged commit 089b84d into twilight-rs:next Jul 1, 2023

vilgotf linked an issue Jul 1, 2023 that may be closed by this pull request

Support all Select Menu types #2047

Closed

archer-321 mentioned this pull request Jul 28, 2024

Support all Select Menu types #2047

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(twilight-model)!: Implement additional select menu types #2219

feat(twilight-model)!: Implement additional select menu types #2219

archer-321 commented Jun 12, 2023 •

edited

Loading

baptiste0928 left a comment

archer-321 commented Jun 13, 2023

baptiste0928 commented Jun 13, 2023

archer-321 commented Jun 14, 2023

HTGAzureX1212 left a comment

Gelbpunkt left a comment

HTGAzureX1212 commented Jun 14, 2023 •

edited

Loading

Gelbpunkt commented Jun 14, 2023

ImDarkDiamond commented Jun 14, 2023

HTGAzureX1212 left a comment

archer-321 commented Jun 15, 2023

laralove143 commented Jun 15, 2023

HTGAzureX1212 commented Jun 16, 2023

laralove143 commented Jun 16, 2023

archer-321 commented Jun 20, 2023

HTGAzureX1212 commented Jun 21, 2023

archer-321 commented Jun 21, 2023

baptiste0928 commented Jun 23, 2023

archer-321 commented Jun 24, 2023 •

edited

Loading

Gelbpunkt left a comment

Gelbpunkt Jun 25, 2023

archer-321 Jun 25, 2023

Gelbpunkt Jul 1, 2023

HTGAzureX1212 Jul 1, 2023

vilgotf commented Jul 1, 2023 •

edited

Loading

Gelbpunkt commented Jul 1, 2023

HTGAzureX1212 commented Jul 1, 2023

vilgotf commented Jul 1, 2023

feat(twilight-model)!: Implement additional select menu types #2219

feat(twilight-model)!: Implement additional select menu types #2219

Conversation

archer-321 commented Jun 12, 2023 • edited Loading

General idea

Breaking changes

Related work

Tests

Signed commits

Labels

baptiste0928 left a comment

Choose a reason for hiding this comment

archer-321 commented Jun 13, 2023

baptiste0928 commented Jun 13, 2023

archer-321 commented Jun 14, 2023

HTGAzureX1212 left a comment

Choose a reason for hiding this comment

Gelbpunkt left a comment

Choose a reason for hiding this comment

HTGAzureX1212 commented Jun 14, 2023 • edited Loading

Gelbpunkt commented Jun 14, 2023

ImDarkDiamond commented Jun 14, 2023

HTGAzureX1212 left a comment

Choose a reason for hiding this comment

archer-321 commented Jun 15, 2023

laralove143 commented Jun 15, 2023

HTGAzureX1212 commented Jun 16, 2023

laralove143 commented Jun 16, 2023

archer-321 commented Jun 20, 2023

HTGAzureX1212 commented Jun 21, 2023

archer-321 commented Jun 21, 2023

baptiste0928 commented Jun 23, 2023

archer-321 commented Jun 24, 2023 • edited Loading

Gelbpunkt left a comment

Choose a reason for hiding this comment

Gelbpunkt Jun 25, 2023

Choose a reason for hiding this comment

archer-321 Jun 25, 2023

Choose a reason for hiding this comment

Gelbpunkt Jul 1, 2023

Choose a reason for hiding this comment

HTGAzureX1212 Jul 1, 2023

Choose a reason for hiding this comment

vilgotf commented Jul 1, 2023 • edited Loading

Gelbpunkt commented Jul 1, 2023

HTGAzureX1212 commented Jul 1, 2023

vilgotf commented Jul 1, 2023

archer-321 commented Jun 12, 2023 •

edited

Loading

HTGAzureX1212 commented Jun 14, 2023 •

edited

Loading

archer-321 commented Jun 24, 2023 •

edited

Loading

vilgotf commented Jul 1, 2023 •

edited

Loading