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

feat: add interaction support to args #728

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

samfundev
Copy link
Contributor

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 a ChatInputParser.

The biggest problem with the current implementation is that .many doesn't map very well to slash commands. Would love any suggestions.

Resolves #494

@favna
Copy link
Member

favna commented Feb 28, 2024

@vladfrangu please review this PR (as discussed in DMs)

Copy link
Member

@vladfrangu vladfrangu left a 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!)

@samfundev
Copy link
Contributor Author

@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.

src/lib/parsers/Args.ts Outdated Show resolved Hide resolved
src/lib/parsers/ChatInputCommandArgs.ts Outdated Show resolved Hide resolved
src/lib/parsers/ChatInputCommandArgs.ts Outdated Show resolved Hide resolved
src/lib/parsers/ChatInputCommandArgs.ts Outdated Show resolved Hide resolved
src/lib/parsers/ChatInputCommandArgs.ts Outdated Show resolved Hide resolved
src/lib/parsers/MessageArgs.ts Outdated Show resolved Hide resolved
src/lib/structures/Argument.ts Outdated Show resolved Hide resolved
src/lib/structures/Command.ts Outdated Show resolved Hide resolved
src/lib/structures/Command.ts Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
@vladfrangu
Copy link
Member

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! ❤️

@samfundev
Copy link
Contributor Author

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.

Copy link
Member

@vladfrangu vladfrangu left a 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

src/lib/parsers/Args.ts Outdated Show resolved Hide resolved
@vladfrangu
Copy link
Member

@sapphiredev pack this

@sapphiredev
Copy link

sapphiredev bot commented Mar 12, 2024

Heya @vladfrangu, I've started to run the deployment workflow on this PR at 68d8c87. You can monitor the build here!

@vladfrangu
Copy link
Member

Packed under @sapphire/framework@6.0.0-pr-728.68d8c87.0

@samfundev
Copy link
Contributor Author

Also, should I be marking conversations as resolved or do you wish to do that?

@vladfrangu
Copy link
Member

Also, should I be marking conversations as resolved or do you wish to do that?

Feel free to mark any resolved convo as resolved

@vladfrangu
Copy link
Member

@sapphiredev pack this

@sapphiredev
Copy link

sapphiredev bot commented Mar 14, 2024

Heya @vladfrangu, I've started to run the deployment workflow on this PR at d435f5c. You can monitor the build here!

@vladfrangu
Copy link
Member

@sapphiredev pack

@sapphiredev
Copy link

sapphiredev bot commented Mar 17, 2024

Heya @vladfrangu, I've started to run the deployment workflow on this PR at 524e708. You can monitor the build here!

@samfundev
Copy link
Contributor Author

@vladfrangu Any updates about this? I'm still not sure if my implementation of many() makes sense.

@favna
Copy link
Member

favna commented Aug 26, 2024

@vladfrangu Any updates about this?

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.

request: update Args to support Chat Input commands
3 participants