Skip to content

provides fix for #219#220

Open
mcdaddytalk wants to merge 1 commit into
AlexzanderFlores:mainfrom
mcdaddytalk:main
Open

provides fix for #219#220
mcdaddytalk wants to merge 1 commit into
AlexzanderFlores:mainfrom
mcdaddytalk:main

Conversation

@mcdaddytalk
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown
Contributor

@tippfehlr tippfehlr left a comment

Choose a reason for hiding this comment

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

I didn't test it but this seems good.

We have to think about converting all args to strings though: i find it annoying to use const foo = args[n] === 'true' and getBoolean() from discord.js is easier I think.

Comment thread src/SlashCommands.ts
options.data.forEach(({ value }) => {
args.push(String(value))
})
for (let option of interaction.options.data) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use a const here

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Easy enough to change. Will submit new commit

Comment thread src/SlashCommands.ts
})
for (let option of interaction.options.data) {
if (option.type === 'SUB_COMMAND' || option.type === 'SUB_COMMAND_GROUP') {
option.options?.forEach(({ value }) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't like to mix forEach and for in loops but that's personal preference

@mcdaddytalk
Copy link
Copy Markdown
Author

agreed on questioning converting all args to strings.
line 92: const args: String[] = [] would need to be generic typed to allow for other data types
I can work that out and submit new commit

@tippfehlr
Copy link
Copy Markdown
Contributor

it's a breaking change so maybe we name it differently until args is deprecated. Isn't necessary though.

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