Open kangalio opened 1 year ago
We cannot just deem certain enums as "oh well Discord will never add a variant" and have two different styles of enum based on that assumption. The breaking can be solved with non_exhaustive
in the existing system or we should convert all enums to structs (with terrible usablity and massively bloating the "enum" sizes).
We cannot just deem certain enums as "oh well Discord will never add a variant"
I said "Some enums won't realistically get new fields in existing variants".
The breaking can be solved with
non_exhaustive
in the existing system
The breaking of adding new variants can be solved with #[non_exhaustive]
, but the breaking of adding new fields to existing variants can't, really.
It is possible to annotate individual enum variants with #[non_exhaustive]
to mean that the individual variants may gain new fields, however the variants become unconstructable then. Actually, this is probably okay for CommandDataOptionValue. But it's not okay for ButtonKind, Action, and Trigger.
Ah okay, would it not be fixed by splitting the enum struct fields into their own structs, then referencing them in the enums? Then a new function or something could be used instead of literal syntax
Thinking about it, that could be automated with a macro
Ah okay, would it not be fixed by splitting the enum struct fields into their own structs, then referencing them in the enums? Then a new function or something could be used instead of literal syntax
That would indeed be another solution. I'm kinda opposed to that because it would add a big pile of overhead on top of the existing overhead of the enum approach.
Thinking about it, that could be automated with a macro
True, at least the new overhead from TriggerKeyword, TriggerHarmfulLink, TriggerSpam, and TriggerKeywordPreset. I'll try something
By the way, the manual Serialize/Deserialize could also be automated with serde-derive, if dtolnay would just stop ignoring and finally merge https://github.com/serde-rs/serde/pull/2056 :sob:
I'll try something
The following macro works for the current state of Trigger (all fields required)
However, if Discord adds a new optional field, the macro would add it to the new()
constructor fields automatically. So the macro would need to differentiate between required and optional fields. This is messy; I think it's only possible with tt-munching or a proc macro
We currently sometimes use enums to model mutually exclusive fields, which is very good usability.
Examples
https://github.com/serenity-rs/serenity/blob/fd0b5734ce173425b4653cfcc01628b17e9638b2/src/model/application/component.rs#L97-L100 https://github.com/serenity-rs/serenity/blob/fd0b5734ce173425b4653cfcc01628b17e9638b2/src/model/guild/automod.rs#L82-L88 https://github.com/serenity-rs/serenity/blob/fd0b5734ce173425b4653cfcc01628b17e9638b2/src/model/guild/automod.rs#L274-L290 https://github.com/serenity-rs/serenity/blob/fd0b5734ce173425b4653cfcc01628b17e9638b2/src/model/application/command_interaction.rs#L629-L643However, if Discord adds a new field to one of the variants, that's a breaking change. This has happened with Action::BlockMessage:
https://github.com/serenity-rs/serenity/blob/fd0b5734ce173425b4653cfcc01628b17e9638b2/src/model/guild/automod.rs#L274-L290
To be forward-compatible with new fields, we should change back some of those enums to structs. Some enums won't realistically get new fields in existing variants, like CommandDataOptionValue; we can leave those as enums.
In the case of Action, the equivalent struct would look like