-
Notifications
You must be signed in to change notification settings - Fork 82
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
Serialize unavailable guilds properly #675
Conversation
public data class GuildCreate(val guild: DiscordGuild, override val sequence: Int?) : DispatchEvent() | ||
public data class GuildCreate(val guild: DiscordPartialGuild, override val sequence: Int?) : DispatchEvent() |
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.
I'm not sure if we should use DiscordPartialGuild
for this. This rather seems to be a union of DiscordUnavailableGuild
(which represents this) and DiscordGuild
. We might instead introduce a sealed superclass for these two and decide on whether to deserialize it as a DiscordGuild
or a DiscordUnavailableGuild
based on the presence and true
value of the unavailable
field.
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.
So something like this:
// supertype for DiscordUnavailableGuild and DiscordGuild
@Serializable(with = DiscordGuildCreateGuildSerializer::class)
public sealed interface DiscordGuildCreateGuild {
public val id: Snowflake
}
private object DiscordGuildCreateGuildSerializer : JsonContentPolymorphicSerializer<DiscordGuildCreateGuild>(DiscordGuildCreateGuild::class) {
override fun selectDeserializer(element: JsonElement) =
when (element.jsonObject["unavailable"]?.jsonPrimitive?.boolean) {
true -> DiscordUnavailableGuild.serializer()
false, null -> DiscordGuild.serializer()
}
}
DiscordGuild.unavailable
could then also be removed.
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.
This at least works for the available case in the basic test I did (unavailable is hard to reproduce).
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.
do we even need a super type 🤔
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.
I mean what should we put in the event class if there is no supertype? We don't have union types.
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.
this also supports my suggestion: discord/discord-api-docs@745685b
fixes #627