Skip to content
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

Implement polls #933

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Implement polls #933

wants to merge 27 commits into from

Conversation

DRSchlaubi
Copy link
Member

No description provided.

@Moortu
Copy link

Moortu commented Aug 18, 2024

Would love to see this feature merged after a review, conflicts resolved and fixes

@lukellmann
Copy link
Member

Would love to see this feature merged after a review, conflicts resolved and fixes

i can take a look, but i'll be on vacation for a week first

@Moortu
Copy link

Moortu commented Aug 19, 2024

Would love to see this feature merged after a review, conflicts resolved and fixes

i can take a look, but i'll be on vacation for a week first

Small world, I'm on vacation rn for a week

Have a good week.
And I look forward to this feature.

I can upgrade my bot in a significant way with it.

@DRSchlaubi
Copy link
Member Author

Now ready

DRSchlaubi and others added 5 commits August 25, 2024 13:24
They seem to be here by accident, I already removed them for good in
#944.
All other kord enums use camel case naming.
Copy link
Member

@lukellmann lukellmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also missing Permission.SendPolls, Intent.GuildMessagePolls, Intent.DirectMessagePolls

@@ -308,7 +308,8 @@ public data class InteractionCallbackData(
@SerialName("component_type")
val componentType: Optional<ComponentType> = Optional.Missing(),
val values: Optional<List<String>> = Optional.Missing(),
val components: Optional<List<DiscordComponent>> = Optional.Missing()
val components: Optional<List<DiscordComponent>> = Optional.Missing(),
val poll: Optional<DiscordPoll> = Optional.Missing()
Copy link
Member

@lukellmann lukellmann Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
val poll: Optional<DiscordPoll> = Optional.Missing()

InteractionCallbackData models https://discord.com/developers/docs/interactions/receiving-and-responding#interaction-object-interaction-data, there is no poll object there (it would be included in DiscordInteraction.message (for components on a poll message, if supported at some point) or InteractionCallbackData.resolved.messages (for message commands etc.) instead)

/**
* Behavior of a Poll message.
*/
public interface PollBehavior : MessageBehavior {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think it's a good idea to have Poll/PollBehavior extend Message/MessageBehavior - a poll is part of a message, not a message itself; it doesn't have a message type; ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am quite sure poll message can't contain anything but the poll

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that might be true for now, but who knows about the future. conceptually a poll is part of a message.

Comment on lines +19 to +31
/**
* Requests to create a poll.
*
* @throws [RestRequestException] if something went wrong during the request.
*/
@OptIn(KordUnsafe::class)
public suspend inline fun PollParentChannelBehavior.createPoll(block: PollBuilder.() -> Unit): Poll {
contract {
callsInPlace(block, InvocationKind.EXACTLY_ONCE)
}

return createMessage { poll(block) } as Poll
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also iirc the poll function is unsafe, also this returns a poll object

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also iirc the poll function is unsafe, also this returns a poll object

see my other comment about that annotation

*
* @see createPoll
*/
public interface PollParentChannelBehavior : MessageChannelBehavior
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there MessageChannelBehaviors that can't have polls? or what is the reason for this interface?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

announcement channels, I think, not sure but there was one

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can create one in an announcement channel, that can't be it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've checked again i can create polls in all types of channels that would be a MessageChannelBehavior in kord: GUILD_TEXT, DM, GUILD_VOICE, GROUP_DM, GUILD_ANNOUNCEMENT, ANNOUNCEMENT_THREAD, PUBLIC_THREAD, PRIVATE_THREAD, GUILD_STAGE_VOICE

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gdude2002 what channel did you say it wouldn't work in

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

News channels, but it seems they've fixed it now.

@@ -236,7 +245,8 @@ public data class DiscordMessage(
val components: Optional<List<DiscordComponent>> = Optional.Missing(),
val interaction: Optional<DiscordMessageInteraction> = Optional.Missing(),
val thread: Optional<DiscordChannel> = Optional.Missing(),
val position: OptionalInt = OptionalInt.Missing
val position: OptionalInt = OptionalInt.Missing,
val poll: Optional<DiscordPoll> = Optional.Missing()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should also be added to DiscordPartialMessage to be safe


override fun toRequest(): CreatablePoll = CreatablePoll(
question ?: error("Please set a question"),
answers,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
answers,
answers.toList(),

defensive copy

Comment on lines +84 to +113
public fun answer(title: String, emojiUnicode: String? = null, id: Int = answers.size) {
require(answers.size < 10) { "Cannot add more than 10 answers" }
answers.add(
DiscordPoll.Answer(
answerId = id,
pollMedia = DiscordPoll.Media(
Optional(title),
Optional(emojiUnicode?.let { DiscordPartialEmoji(name = it) }).coerceToMissing()
)
)
)
}

/**
* Adds an answer with [title] and [emoji].
*
* @param id the answer id
*/
public fun answer(title: String, emoji: Snowflake? = null, id: Int = answers.size) {
require(answers.size < 10) { "Cannot add more than 10 answers" }
answers.add(
DiscordPoll.Answer(
answerId = id,
pollMedia = DiscordPoll.Media(
Optional(title),
Optional(emoji?.let { DiscordPartialEmoji(id = it) }).coerceToMissing()
)
)
)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the id required to be sent? that is is the answer_id field optional on poll creation? this makes me think it might be:

Only sent as part of responses from Discord's API/Gateway.

*
* @param id the answer id
*/
public fun answer(title: String, emojiUnicode: String? = null, id: Int = answers.size) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could emoji be used for the parameter name instead?

Comment on lines +202 to +203
"MESSAGE_POLL_VOTE_ADD" -> MessagePollVoteAdd(decode(MessagePollEventData.serializer()), sequence)
"MESSAGE_POLL_VOTE_REMOVE" -> MessagePollVoteRemove(decode(MessagePollEventData.serializer()), sequence)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd keep these above the deprecated events

Comment on lines +652 to +653
public data class MessagePollVoteAdd(val data: MessagePollEventData, override val sequence: Int?) : DispatchEvent()
public data class MessagePollVoteRemove(val data: MessagePollEventData, override val sequence: Int?) : DispatchEvent()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these should also be exposed in core

@lukellmann
Copy link
Member

also relevant: discord/discord-api-docs#7090

@lukellmann
Copy link
Member

also discord/discord-api-docs#7050

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants