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

Managed application commands #454

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

Conversation

portasynthinca3
Copy link

This pull request is intended to reduce boilerplate in projects that use application commands. Instead of requiring library users to implement a Consumer and manually handle INTERACTION_CREATE events, this pull requests enables them to define a separate module per command containing the command specification and its handling function.

As a bonus, it removes magic numbers in command specs, instead opting in for atoms, and merges some spec fields into one field (for example, name and name_localizations are represented as a tuple and value_min and value_max as a range)

@jchristgit jchristgit self-assigned this Mar 13, 2023
@jchristgit jchristgit self-requested a review March 13, 2023 19:22
Copy link
Collaborator

@jchristgit jchristgit left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, and sorry for the late review!

This looks mostly good. It suffers from one problem: when an event is received, it's passed to a random consumer, due to the way we're using GenStage at the moment.

I think a better solution than to create an internal consumer would be to simply instruct users to call handle_event or maybe just a function handle_interaction that is called directly with the received interaction.

@jchristgit
Copy link
Collaborator

Hi @portasynthinca3!

On master we removed GenStage in favour of :pg. In theory your usage of making a new consumer works with it, but in practice I think it's more straightforward for users to simply have an API they forward :INTERACTION_CREATE events in their consumer to.

What do you think?

@portasynthinca3
Copy link
Author

Hi! Sorry for not replying earlier. I have a couple of design decisions that i need but cannot make:

  1. Does this belong in the main library? When I originally presented my PR in the unofficial Discord API server, people there suggested that I instead make it into a separate library, so I started working on that. It even included a powerful but not-fully-complete localization engine. I stopped working on it because I kind of stopped programming for a while, but I will for certain return to it later.
  2. While I do agree that users passing :INTERACTION_CREATE events to this component of the main library or my separate library, whatever shape my idea ends up taking, is more "Elixir-ian" and more true to FP dogmas, I also think that automatic handling of the events will result in less boilerplate on the user side

So, what do you think?

@jchristgit
Copy link
Collaborator

Does this belong in the main library? When I originally presented my PR in the unofficial Discord API server, people there suggested that I instead make it into a separate library, so I started working on that. It even included a powerful but not-fully-complete localization engine. I stopped working on it because I kind of stopped programming for a while, but I will for certain return to it later.

Hmm this is a good point. Having it separately is also how I do it in my command library and it works well. I wouldn't mind accepting it into the main library giving that Discord's builtin slash commands have gotten relatively good, and should help remove much boilerplate when writing commands, but I do understand Nostrum is still relatively low-level so I agree that a separate library would be better.

While I do agree that users passing :INTERACTION_CREATE events to this component of the main library or my separate library, whatever shape my idea ends up taking, is more "Elixir-ian" and more true to FP dogmas, I also think that automatic handling of the events will result in less boilerplate on the user side

I am a fan of reducing boilerplate, but in this case, as we currently have no way of limiting consumers to specific events, it means effectively duplicating all gateway events to the app's consumer and this one. If your bot is big enough this can cause considerable load increase, and I think we should strive to keep it relatively efficient.

@jchristgit jchristgit assigned Kraigie and jb3 and unassigned jchristgit Sep 11, 2023
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.

None yet

4 participants