-
Notifications
You must be signed in to change notification settings - Fork 20
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 Support For Slash Commands #73
Conversation
ooooo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if I have any permission to review this project (not sure if you need to even follow any of this), but I would still like to give my advice.
I have been pushing back on adding slash commands just because of how limiting they are, but I guess it's fine for the to be pushed for now. I appreciate the work you put into this and with sam quotes.
Here is my advice here, generally the idea I had in mine for slash commands was not for slash commands to be "messily" integrated into the current codebase. You have done well at that, as you have abstracted lots of the reply classes which was something that is quite perfect. The only thing here we need to try to make a little nicer here is the argument parsing, as I feel we can integrate it a little bit more abstractly.
Thank you, yet again. :)
src/main/java/com/diamondfire/helpbot/bot/command/CommandHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/com/diamondfire/helpbot/bot/command/CommandHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/com/diamondfire/helpbot/bot/events/commands/SlashCommandEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/com/diamondfire/helpbot/sys/multiselector/MultiSelector.java
Outdated
Show resolved
Hide resolved
src/main/java/com/diamondfire/helpbot/sys/multiselector/MultiSelector.java
Outdated
Show resolved
Hide resolved
src/main/java/com/diamondfire/helpbot/sys/multiselector/MultiSelectorBuilder.java
Outdated
Show resolved
Hide resolved
I fixed a lot of the issues, but the major changes might have to wait until soon™. |
Take your time here, this is a pretty major change and it's important we get this right. :) There is no rush here |
Ye I'm on Thanksgiving break now so lots of time to program stuff. |
Converting to draft due to changes in #80 that I'd like to merge into here. |
Merged from #80. This mostly works, except for command permissions, which may require an upgrade to JDA v5 and some additional considerations. Additional considerations include possibly moving to the mardadb jdbc connector and updating the action dump, which I have updated to a newer version of mcprotocollib but have not moved to msa. |
@RedDaedalus any thoughts on how we should continue here? |
I'm going to close this for now since it's become somewhat of a world plots moment but some stuff will probably make it into some smaller prs. |
This pull request includes slash command support and a LOT of refactoring.
Full List of Changes
EndlessStringArgument
asGreedyStringArgument
(formerlyMessageArgument
) was identical.FollowupReplyHandler
system.Updating
Config Changes
The following entries have been added to the config. (This can be copy-pasted into an existing config file.)
Other