Change from many create_*_application_command requests to a single bulk_overwrite_*_application_commands request #34
Conversation
|
I'm quite new to contributing to open source, so any and all comments are welcome. One potential point I can see being brought up is rather than making it a breaking change, it could be more feasible to add it as an alternative registration flow alongside the existing add_command flow. (Edit: I've now implemented this change in a separate branch which I will merge into this one if this is something desirable) |
…on-optional Restore former add_command functionality
|
Made the queue command flow optional and turned this into a non-breaking change by restoring the add_command functionality. |
|
Thank you very much for the PR! |
jchristgit
added
enhancement
…on-optional Rename handle callbacks to better reflect actions
|
Fixed the lint errors and renamed the callback atoms. I think that should fix the failed lint workflow. No rush, just didn't want it forgotten. Now I know it's been looked at, feel free to take your time! |
| @@ -92,13 +92,27 @@ defmodule Nosedrum.Storage do | |||
| ## Return value | |||
| Returns `:ok` if successful, and `{:error, reason}` otherwise. | |||
| """ | |||
| @callback add_command( | |||
| @callback queue_command( | |||
There was a problem hiding this comment.
We should update the documentation for this to read that it adds the command to the internal dispatch storage, but does not yet send it to Discord, and also that process_queued_commands should be used once all desired commands have been added.
There was a problem hiding this comment.
I'd be happy to. I'm new to contributing to open source, and relatively new to elixir compared to you guys, so if there's any steps other than just updating the doc comments, let me know.
There was a problem hiding this comment.
Nope, just updating the doc comment would be enough 🙂
| @doc """ | ||
| Add a new command under the given name or application command path. | ||
|
|
||
| If the command already exists, it will be overwritten. |
There was a problem hiding this comment.
Should we document here that when many commands are being registered, it is recommended to use queue_command and process_queue_commands instead?
There was a problem hiding this comment.
Yes, good idea.
kshannoninnes
force-pushed
the
feature/bulk-register-commands
branch
from
bb7146f to
1aaf165
Compare
|
I think that's everything commented on, should be good to go a final round against the linter lol |
Problem
Nosedrum currently makes one request to Nostrum.Api.create_{guild, global}_application_command for each command being "added". This would be fine if Nosedrum's default storage "Dispatcher" had a way of keeping track of commands while the bot is not running (eg. a database or some other external storage). As it stands, if a bot has to restart it loses all links to the Discord commands. While Discord still maintains the existing commands, the only way to re-link them to the command modules is by re-registering them.
Proposed Solution
Discord (and Nostrum) provide an alternative endpoint to avoid running into rate limits when re-registering commands. By using the bulk_overwrite_*_application_command endpoints we're able to link the existing commands on Discord's side with the modules on our application's side, without running into the 200 per guild daily rate limit applied to the create endpoint.
Essentially it's resorting to a sort of builder pattern. We add all the commands to Nosedrum's command map before calling update_discord/2 which makes a single call to bulk_overwrite_*_application_command.
This Is A Breaking Change
The change from add_command actually sending commands to Discord to simply queueing them up until update_discord/2 is called means that any command registration in existing applications needs to be updated to call update_discord/2 once they have added all their commands.
I'm unaware of how other bot developers implement their command registration when using Nosedrum, so it may also be necessary for bots to perform this add/update step on startup.
Upsides
Downsides