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

Slash Commands, Message Components, and Interactions #31

Open
wants to merge 47 commits into
base: master
Choose a base branch
from

Conversation

soya-daizu
Copy link
Contributor

@soya-daizu soya-daizu commented Oct 6, 2021

EDIT(Oct. 2022): I'm still around and the branch is still being maintained with support of the newer interaction related features. Please report any issues if you found any!

Now that #19 and #20 are being kinda dead, here's my attempt to implement all these features. The added lines are about a quarter of that in #19 and #20 combined, and while some part of the code may not be the best, at least I will try to be active as I can and address any problems pointed out.
This is the first time for me to implement all this stuff while reading docs, and the changes ended up pretty big still, so I would like to have some feedback first.

Some notes about the changes:

  • You can ignore most of the changes in mappings/channel.cr as they're from Allowed Mentions #30
  • There are some shorthand methods that can be used when declaring command structures
    • I know that the lib is supposed to be minimal but the names are already getting long and it's simply verbose, so this is providing some ways to make it shorter
  • PartialApplicationCommand
    • Used only for batch registering commands
    • Does not have id and other fields that are not used when registering
  • About the changes in PartialGuildMember
    • Partial guild member objects sent in Resolved Interaction Data does not contain deaf and mute fields

Copy link
Contributor

@greenbigfrog greenbigfrog left a comment

Choose a reason for hiding this comment

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

Some thoughts I noticed during a first quick glance.

examples/interactions.cr Outdated Show resolved Hide resolved
src/discordcr/mappings/application_commands.cr Outdated Show resolved Hide resolved
@soya-daizu
Copy link
Contributor Author

Working on to add support for permissions v2, but I'm not sure how I should implement REST#edit_application_command_permissions.
Instead of the Bot token, it now requires Bearer token of the user who has access to manage the guild.
In other cases that require a token of the user, such as #add_guild_member, we can just pass the token in the json body, but this time it seems like the token has to be specified in the Authorization header.
Should we change the #request method to take an optional token value, or let the lib users to create another Client with the user's token just so that they can use #edit_application_command_permissions?

Relevant doc: https://discord.com/developers/docs/interactions/application-commands#edit-application-command-permissions

@PixeLInc
Copy link
Member

Working on to add support for permissions v2, but I'm not sure how I should implement REST#edit_application_command_permissions. Instead of the Bot token, it now requires Bearer token of the user who has access to manage the guild. In other cases that require a token of the user, such as #add_guild_member, we can just pass the token in the json body, but this time it seems like the token has to be specified in the Authorization header. Should we change the #request method to take an optional token value, or let the lib users to create another Client with the user's token just so that they can use #edit_application_command_permissions?

Relevant doc: discord.com/developers/docs/interactions/application-commands#edit-application-command-permissions

The request method already takes headers as an argument.
The raw_request method can just be changed to something like this:

diff --git a/src/discordcr/rest.cr b/src/discordcr/rest.cr
index 5fcb123..6ef619d 100644
--- a/src/discordcr/rest.cr
+++ b/src/discordcr/rest.cr
@@ -22,7 +22,9 @@ module Discord
       mutexes = (@mutexes ||= Hash(RateLimitKey, Mutex).new)
       global_mutex = (@global_mutex ||= Mutex.new)

-      headers["Authorization"] = @token
+      unless headers["Authorization"]?
+        headers["Authorization"] = @token
+      end
       headers["User-Agent"] = USER_AGENT
       headers["X-RateLimit-Precision"] = "millisecond"

and then within the edit_application_permissions method, just pass in a custom authorization header with a bearer token.

@soya-daizu
Copy link
Contributor Author

Oh, you're right. Seems like I just didn't read the code.
Thank you for pointing it out!

examples/interactions.cr Outdated Show resolved Hide resolved
src/discordcr/rest.cr Show resolved Hide resolved
@badBlackShark
Copy link
Member

Hi :) Sorry that it's been so long since there's been any activity! Would it be possible to add a couple of specs for this? Otherwise, I didn't see any issues with the code, but I'm also the most junior developer on hand, so take that with a grain of salt.

@soya-daizu
Copy link
Contributor Author

Hi, thanks for taking your time to look through my PR. And yes, I can definitely add some specs for it, but I can't exactly tell when it will be ready. I think I will have some time by the end of this month, so hopefully it will be ready in the next month or so.

@badBlackShark
Copy link
Member

Hi, thanks for taking your time to look through my PR. And yes, I can definitely add some specs for it, but I can't exactly tell when it will be ready. I think I will have some time by the end of this month, so hopefully it will be ready in the next month or so.

Awesome! I mean, after the response time to your PR taking a couple weeks is definitely no problem! Thanks for your support for the library! :)

@soya-daizu
Copy link
Contributor Author

I've added some specs for the structs that require custom json parsing, please let me know if there's anything missing!

Copy link
Member

@badBlackShark badBlackShark left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

@badBlackShark
Copy link
Member

@PixeLInc do you think you'll have time to review this at some point? I don't feel confident/knowledgeable enough to truly approve this.

@PixeLInc PixeLInc mentioned this pull request Apr 2, 2023
@watzon
Copy link

watzon commented Apr 2, 2023

Everything seems to be good and functional. Any idea why I'd get a 401 when trying to bulk overwrite global application commands though?

@soya-daizu
Copy link
Contributor Author

Thanks for looking through the PR!
I first thought that it might be a regression caused by the most recent commit, but that one shouldn't cause any issues.. I know you already have checked it, but have you made sure the token is set properly? The token also needs to be prefixed with Bot .

@watzon
Copy link

watzon commented Apr 3, 2023

@soya-daizu I knew it had to be something stupid like that. Of course adding the Bot bot before the token works. Maybe we could do something about that? Otherwise, great job! Everything works as expected.

@xhyrom
Copy link

xhyrom commented Apr 6, 2023

@shardlab

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.

9 participants