Support all Select Menu types

[ciphynx]

Select Menus now support 5 different types:

• String Select 3 - Only type that supports & requires the options field currently.
• User Select 5
• Role Select 6
• Mentionable Select 7
• Channel Select 8 - Only type that supports the optional channel_types field.

All but the String Select type supports and returns resolved data in interaction payload.

Reference: Discord Docs

[cptpiepmatz]

I'll give this a shot!

[cptpiepmatz]

it seems that #2051 will solve this

[archer-321]

@zeylahellyer, sorry if this is not the right place, but I figured it's best to continue discussing potential implementations here now that the initial implementation PR is closed.

To clarify, when you mention making data an enum in this comment, do you mean extracting all fields that are unique to a specific type of select menu as outlined in the example below? If so, that would indeed combine the benefits of a SelectMenu enum with a flattened struct to avoid duplicating some fields. Moreover, I believe this is the closest a (reasonable) implementation will get to specialization in this case.

pub struct SelectMenu {
    pub custom_id: String,
    pub disabled: bool,
    pub max_values: Option<u8>,
    pub min_values: Option<u8>,
    pub placeholder: Option<String>,
    // The (de-)serialization would, of course, flatten the enum below
    pub data: SelectMenuData,
}

pub enum SelectMenuData {
    Text {
        options: Vec<SelectMenuOption>,
    },
    Channel {
        channel_types: Option<Vec<ChannelType>>,
    }
    // ...
}

Considering this update would also move the existing options to a new enum variant (and out of the structure), would this be too big of a breaking change? I know Twilight is still pre-1.0, but considering a lot of bots will be affected by this, it might be worth documenting the required changes somewhere (e.g. in the changelog).

[cptpiepmatz]

I would like to help implementing this if anyone here needs any

[laralove143]

i'd prefer that the enums wrapped over structs instead of being enum structs, something like

pub enum SelectMenuData {
    Text(Vec<SelectMenuOption>),
    Channel(Option<Vec<ChannelType>>),
    // ...
}

if theres only one field, or like https://api.twilight.rs/twilight_model/application/interaction/enum.InteractionData.html if theres multiple, its more versatile that way

[archer-321]

i'd prefer that the enums wrapped over structs instead of being enum structs, something like

pub enum SelectMenuData {
    Text(Vec<SelectMenuOption>),
    Channel(Option<Vec<ChannelType>>),
    // ...
}

if theres only one field, or like https://api.twilight.rs/twilight_model/application/interaction/enum.InteractionData.html if theres multiple, its more versatile that way

In addition to moving multi-field structs into separate structs, I'd consider boxing some / most structs, as they could easily increase the enum size for all other components, otherwise (also outlined in the RFC I opened).

There's only a potential problem with API stability if we inline variants that only have a single field (even if we box that). While it doesn't matter with the current approach of having exhaustive structs, It might become necessary to create "constructors" for those structs and mark them as #[non_exhaustive] once Twilight approaches 1.0. Even if that's not going to matter soon, the approach we choose here should allow for maximum API stability if we plan to apply it to other parts of the library as well.

[laralove143]

making a struct #[non_exhaustive] means users will no longer be able to create it (if all of its fields are pub) so we should consider if we want the users to not be able to do that, we could have a public new method for that but then the changes in the new method would be breaking

[archer-321]

Indeed, that's why I suggested it might be required in the future. Without marking a struct as non-exhaustive, adding new fields is always a breaking change. I.e., if Discord adds a new parameter within the same API version, twilight would have to go 1.0.0 -> 2.0.0.

Of course, the new method would still require breaking changes for new required parameters, but if the Discord API adds new optional parameters (which should be the default without increasing their own API version), the new (or even Default::default() for types that don't have any required fields at all) would provide the necessary API stability.

Again, this is something for the 1.0.0 milestone, but if this pattern is to be applied throughout the library, it at least shouldn't make things harder than they have to be.

[laralove143]

it makes more sense now, i agree we should mark most structs and enums #[non_exhaustive] and add a new method, this adds quite a bit of boilerplate though, so we might want to skip the new methods on types only returned by gateway, http or cache

alternatively, and this isnt very common but it solves the issue, we can separate required fields (params of new) and optional ones

// exhaustive because dapi shouldnt change these fields
struct Foo {
  required: u8,
  optional: FooOptional,
}

#[non_exhaustive]
#[derive(Default)]
struct FooOptional {
  s: Option<String>,
}

// constructed like
let mut foo = Foo {
  required: 0,
  optional: FooOptional::default(),
};
foo.optional.s = Some("foo".to_owned()); // not breaking because dapi shouldnt remove fields, only add non-required fields (right?)
// or
Foo {
  required: 0,
  optional: FooOptional {
    s: Some("foo".to_owned()),
    ..Default::default()
  },
};

[archer-321]

With #2219 merged, is there anything left for this issue?

[laralove143]

i'll close this assuming no, feel free to reopen if theres anything left