feat(model): initial pass at dealing with unknown enum variants

[AEnterprise]

Reworking enums to have unknown variants to handle discord adding new api features.

Goal: not losing any payloads whenever discord adds something new, while keeping the type safety the enums provide since people depend on that.

This is an initial WIP draft to get discussion going on how the final implementation should look, but is fully functional.

Left to do on this:

• standardize serialization and deserialization tests. they where all over the place, some check serde tokens, others just conversions without serde. We need to decide what kind of test general model serializations and deserializations should have => best left for a seperate PR
• figure out a better solution for interactions, possibly by splitting it into 2 enums (a seperate one used for getting the command list over http, and one for the gateway interaction itself. Where dropping unknown interaction types might be slightly acceptable, since we can't do meaningful deserialization with the current models. Or flattening the entire interaction struct like was done with channels recently. Needs more consideration
• Some models are also used as outgoing http payloads. We need to decide if we want to allow sending payloads with these unknown variants or error on them. Allowing unknowns can be useful in allowing people to leverage new options without a library upgrade. But if this different option also needs another field set the library won't be able to do it, nor can we ensure the value is valid. For the creation of application commands this however wasn't an option due to our custom serializer.

[PyroTechniac]

Just some basic formatting, probably missed a lot.

[AEnterprise]

Thanks, for the formatting i'm aware there is probably a lot more that needs fixing. i chose to not fix them yet for this draft because the design of this isn't fully done yet, and i'm currently looking for feedback on how it should look/work in the end

[spring4175]

I only commented on things once and didn't comment on formatting. Do we still have uses of the serde_repr dependency after this?

[spring4175]

standardize serialization and deserialization tests. they where all over the place, some check serde tokens, others just conversions without serde. We need to decide what kind of test general model serializations and deserializations should have

We don't really need to test the tokens, we're essentially testing serde at that point. We should test our own conversion implementations.

figure out a better solution for interactions, possibly by splitting it into 2 enums (a seperate one used for getting the command list over http, and one for the gateway interaction itself. Where dropping unknown interaction types might be slightly acceptable, since we can't do meaningful deserialization with the current models. Or flattening the entire interaction struct like was done with channels recently. Needs more consideration

I believe we're in the long-term process of separating types between their incoming and outgoing variants. I am in favor of more flattening in general but I don't believe it's in the scope of this PR.

Some models are also used as outgoing http payloads. We need to decide if we want to allow sending payloads with these unknown variants or error on them. Allowing unknowns can be useful in allowing people to leverage new options without a library upgrade. But if this different option also needs another field set the library won't be able to do it, nor can we ensure the value is valid. For the creation of application commands this however wasn't an option due to our custom serializer.

I think if people want to use cutting edge new API features that we don't support they can use manual HTTP request bodies. Type flattening may also aid in this, but again out of scope. We may need to place some sort of priority on investigating the role flattening would have for us in general.

[AEnterprise]

Do we still have uses of the serde_repr dependency after this?

Yes, several gateway and interaction structs still use these.

We don't really need to test the tokens, we're essentially testing serde at that point. We should test our own conversion implementations.

i'll replace the relevant tests with just testing the conversions without serde then

I believe we're in the long-term process of separating types between their incoming and outgoing variants. I am in favor of more flattening in general but I don't believe it's in the scope of this PR.

Agreed, i think we can live with it being not perfect here in what data we give when it's unknown and then clean up these few last spots when things are flattened

[7596ff]

This does need further work, and based on some discussion in Discord, there seems to be a path forward. We should bracket any work on unknown interaction variants for now, in order to solve that problem more correctly at a later date. Also, we should try to build on whatever existing tests there are for the affected types, and follow up later to make them more consistent with each other and more correct. Lastly, I'll be making a quick PR to add the Forum channel type, as a stopgap in a minor release, which will come before a) this PR and b) Erk's work on the entire forum feature later on.

[7596ff]

looking good, a bunch of small consistency nits

[vilgotf]

(Commenting on this one, applis on all).
I'm not sure I like the display implementation being "Unknown", should be the number imo

[AEnterprise]

This actually isn't possible unless i'm missing something. These functions return an &'static str. Converting the number to a string would yield a string that is owned in this scope only and thus can not be returned out of the function.

[vilgotf]

No it's impossible in this method, but I meant for the Display implementation; currently it just calls this method, it would be better to print the integer (+ Unknown)

[AEnterprise]

But we don't have a display implementation for most of these, this looks like the only one that has it. It also would require to duplicate the entire match. It seems better to keep this consistent here, and look at if they should all have display implementations (or if this one should be removed) separately

[vilgotf]

It's as simple as:

fmt(&self, f: &mut Formatter<'_>) -> Result<(), Error> {
    if let Unknown(int) = self {
        Display::fmt(&int, f)
    } else {
        f.write_str(self.name())
    }
}

[vilgotf]

But we don't have a display implementation for most of these, this looks like the only one that has it.

Ah I missed this, maybe it should be removed at a later point then

[7596ff]

I like the idea of moving to a Display implementation later on.

[AEnterprise]

Let's keep it as is for now then to not scope creep this PR, and follow it up to move to display soon after

addressed

[7596ff]

Looks great. Excited for this change and what it makes possible.

[baptiste0928]

Looks good, should we mention in CONTRIBUTING.md that enums must have an Unknown variant where applicable?

[spring4175]

This looks great, thanks for the effort on it! With a rebase is this good to go?