-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
feat: add interaction support to args #728
base: main
Are you sure you want to change the base?
Conversation
@vladfrangu please review this PR (as discussed in DMs) |
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.
For starters, this is more-less what I'd personally want for Args v2: https://vladfrangu.notion.site/Args-v2-183d858b10e64cd6b942e56f1b04a405?pvs=4
You can also split up Argument#run into messageRun and chatInputRun if it'd be better for you (since chat command args also come with a type, we should assert the types match the inputs too!)
@vladfrangu Tried to make args abstract based on that document. I had to fight typescript to get peek to work, but I'm not sure how to do it better. |
Also I cannot express enough with words that I am hella grateful that you're taking on this (quite challenging) issue! Don't let the barrage of requested changes scare you! ❤️ |
Thanks for the kind words! Most of this was just moving stuff to the base class, so it was pretty easy changes. Appreciate that you're taking time to look over it. |
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 will also pack your PR and give it a test soon :D
@sapphiredev pack this |
Heya @vladfrangu, I've started to run the deployment workflow on this PR at 68d8c87. You can monitor the build here! |
Packed under |
Also, should I be marking conversations as resolved or do you wish to do that? |
Feel free to mark any resolved convo as resolved |
@sapphiredev pack this |
Heya @vladfrangu, I've started to run the deployment workflow on this PR at d435f5c. You can monitor the build here! |
@sapphiredev pack |
Heya @vladfrangu, I've started to run the deployment workflow on this PR at 524e708. You can monitor the build here! |
@vladfrangu Any updates about this? I'm still not sure if my implementation of many() makes sense. |
ef5ad75
to
4958305
Compare
@vladfrangu Any updates about this? |
6907e59
to
b36892e
Compare
Super rough PR to take a first stab at adding interaction support to
Args
. Wanted to get some feedback before going any further since I don't know if I'm going in the right direction with this implementation. Basically,Args
stays the same but either wraps lexure or aChatInputParser
.The biggest problem with the current implementation is that
.many
doesn't map very well to slash commands. Would love any suggestions.Resolves #494