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(CommandInteraction): add CommandInteractionOptionResolver #6107

Merged
merged 18 commits into from Jul 18, 2021
Merged

feat(CommandInteraction): add CommandInteractionOptionResolver #6107

merged 18 commits into from Jul 18, 2021

Conversation

memikri
Copy link
Contributor

@memikri memikri commented Jul 13, 2021

Please describe the changes this PR makes and why it should be merged:
This PR creates a new structure, CommandInteractionOptionResolver, which acts as a resolver for CommandInteraction options. This replaces the existing Collection<string, CommandInteractionOption> on CommandInteraction#options with the new class.

The new class will allow for easier usage and validation of command options. It also will serve as an additional check against users having mismatches between their option types and their execution code. This also allows for TypeScript users to access the properties in a cleaner and type-safe way without custom parsers or casting.

Example:

import { Client, Snowflake } from "discord.js";

const client = new Client({ intents: [] });

client.on("ready", async () => {
  const guild = await client.guilds.fetch(process.env.DISCORD_GUILD_ID as Snowflake);
  await guild.commands.create({
    name: "test",
    description: "test",
    options: [
      {
        name: "sub",
        description: "sub",
        type: "SUB_COMMAND",
        options: [
          {
            name: "bar",
            description: "bar",
            type: "INTEGER",
            required: true,
          },
          {
            name: "baz",
            description: "baz",
            type: "USER",
            required: false,
          },
        ],
      },
    ],
  });
});

client.on("interactionCreate", async (interaction) => {
  if (interaction.isCommand() && interaction.commandName === "test") {
    const sub = interaction.options.getSubCommand("sub");
    if (sub) {
      const bar = sub.getInteger("bar", true);
      const baz = sub.getUser("baz");
      await interaction.reply((bar * 2).toString() + " " + baz?.id);
    } else {
      await interaction.reply("Error: Invalid subcommand!");
    }
  }
});

client.login(process.env.DISCORD_TOKEN);

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating
  • This PR changes the library's interface (methods or parameters added)
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

@memikri memikri changed the title refactor(CommandInteraction): create CommandInteractionOptionResolver feat(CommandInteraction): add CommandInteractionOptionResolver Jul 13, 2021
@memikri memikri marked this pull request as ready for review July 13, 2021 00:46
@memikri
Copy link
Contributor Author

memikri commented Jul 13, 2021

@iCrawl iCrawl added this to the Version 13 milestone Jul 13, 2021
src/structures/CommandInteraction.js Outdated Show resolved Hide resolved
src/structures/CommandInteraction.js Outdated Show resolved Hide resolved
src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
Copy link
Contributor

@NotSugden NotSugden left a comment

Choose a reason for hiding this comment

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

suppose you could do [required=false] but I only thought this after I made all the review comments and im lazy

src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
@memikri
Copy link
Contributor Author

memikri commented Jul 13, 2021

This may also close #5880

@ImRodry
Copy link
Contributor

ImRodry commented Jul 14, 2021

Can you make the name parameter in getSubCommand optional? A command has multiple subcommands but only one can be ran at once so it doesn’t make much sense to specify the name of the command since you’re not supposed to know what name that’s gonna be.

@memikri
Copy link
Contributor Author

memikri commented Jul 14, 2021

That's why getSubCommand can't be required. It is as intended. Here is an example:

async function run(interaction: CommandInteraction) {
  const subcommand1 = interaction.options.getSubCommand("sub1");
  const subcommand2 = interaction.options.getSubCommand("sub2");
  if (subcommand1) {
    const subarg1 = subcommand1.getInteger("subarg1");
    // etc.
  } else if (subcommand2) {
    const subarg2 = subcommand2.getUser("subarg2");
    // etc.
  }
}

@monbrey
Copy link
Member

monbrey commented Jul 14, 2021

const subcommand1 = interaction.options.getSubCommand("sub1");
const subcommand2 = interaction.options.getSubCommand("sub2");

I don't think this captures what Rodry meant, nor does it really address #5880 if I still don't immediately know which subcommand was run. Why would I want to try to get both (or more) subcommands - you know it has to be one of them. I just want to get whichever one it is.

const subcommand = interaction.options.getSubCommand();
if (subcommand.name === "sub1") {
  const subarg = subcommand.getInteger("integer");
  // etc.
} else if (subcommand.name === "sub2") {
  const subarg = subcommand.getUser("user");
  // etc.
}

@memikri memikri requested a review from SpaceEEC July 14, 2021 11:54
typings/index.d.ts Show resolved Hide resolved
memikri and others added 2 commits July 15, 2021 16:12
Co-authored-by: Sugden <28943913+NotSugden@users.noreply.github.com>
Copy link
Contributor

@ImRodry ImRodry left a comment

Choose a reason for hiding this comment

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

Collection.get() returns undefined when it doesn't find the specified key so I think this behavior should stay the same

typings/index.d.ts Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
typings/index.d.ts Show resolved Hide resolved
@iCrawl
Copy link
Member

iCrawl commented Jul 16, 2021

I don't agree with changing the interface to return undefined rather than the current null

Copy link
Contributor

@ImRodry ImRodry left a comment

Choose a reason for hiding this comment

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

Actually, following the convo on the discord server let's make group and subcommand properties instead of methods

src/structures/CommandInteractionOptionResolver.js Outdated Show resolved Hide resolved
src/errors/Messages.js Show resolved Hide resolved
typings/index.d.ts Show resolved Hide resolved
typings/index.d.ts Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
@iCrawl iCrawl requested a review from vladfrangu July 17, 2021 16:15
@memikri memikri requested a review from vladfrangu July 18, 2021 05:18
@iCrawl iCrawl merged commit f293132 into discordjs:master Jul 18, 2021
@memikri memikri deleted the commandoptionresolver branch July 18, 2021 18:19
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.

None yet

10 participants