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

Add DSharpPlus' official slash command module. #12

Closed
wants to merge 3 commits into from
Closed

Add DSharpPlus' official slash command module. #12

wants to merge 3 commits into from

Conversation

xKaelyn
Copy link

@xKaelyn xKaelyn commented Oct 1, 2021

No description provided.

Copy link
Owner

@stjohann stjohann left a comment

Choose a reason for hiding this comment

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

Testing your changes, I am afraid just adding SlashCommands module does nothing, see DSharpPlus docs:
https://github.com/DSharpPlus/DSharpPlus/tree/master/DSharpPlus.SlashCommands

You have to actually add new code for it to work. This essentially means that

  1. You have to add code required by the module to work;
  2. You have to test how it works with existing setup of most servers (= without Slash Commands authorisation) given If your bot isn't properly authorized, a 403 exception will be thrown on startup line. I don’t expect all servers to authorise Slash Commands immediately, so smooth transition without breaking the bot is something to consider here.
  3. You have to see whether (and how) /help would work, given current customisations in LocalisedHelpFormatter.cs.
  4. We have to choose which commands need SlashCommands treatment and write new code for it, since it does not seem like it would work with code for CommandsNext commands. Preferably this should be done without duplicating it too much.

All of this seems really complicated, and I have stated before that slash commands do not seem like a big priority to me given that the bot only shows 2 commands to regular users. If you wish to continue, that would be fine, but I can only review the changes when needed.

As it stands, in my tests this PR does not resolve #11. Nonetheless, I am glad to see your pull request.

- package-ecosystem: "nuget"
directory: "/"
schedule:
interval: "daily"
Copy link
Owner

Choose a reason for hiding this comment

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

On a related note, I am somewhat sceptical that adding Dependabot would be helpful (since any suggested updates would be mostly non-critical). While no changes in your pull request are required because I can always remove it later, a pull request should generally try to fit a specific purpose (specified by the PR title) so it is easier for maintainers to manage.

@xKaelyn
Copy link
Author

xKaelyn commented Oct 3, 2021

For some reason, I added the necessary code to implement slash commands, even converting a couple commands to use the slash command module, but I don't believe that I remembered to commit my changes. Apologies for that.

@stjohann
Copy link
Owner

stjohann commented Oct 8, 2021

Are they still available for possible review? (Asking mostly as a reminder.)

@xKaelyn
Copy link
Author

xKaelyn commented Oct 8, 2021

Are they still available for possible review? (Asking mostly as a reminder.)

I'm currently away from my computer, should be able to commit them once I'm available.

@stjohann
Copy link
Owner

stjohann commented Dec 3, 2021

Last activity almost 2 months ago. The only substantial change (updating the library) is made now. Please re-open if there are some other commits to check for me here.

@stjohann stjohann closed this Dec 3, 2021
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.

2 participants