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

chore(SlashCommandBuilder): improve message when missing required params #9468

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

wwselleck
Copy link

@wwselleck wwselleck commented Apr 28, 2023

When you try to call toJSON on a SlashCommandBuilder that has not had a name or description set, or has any options that are missing a name or description, you get the following error:

.../node_modules/@sapphire/shapeshift/src/validators/StringValidator.ts:73
                return this.ip(6);
                  ^
Error: Expected a string primitive
    at StringValidator.handle (.../node_modules/@sapphire/shapeshift/src/validators/StringValidator.ts:73:19)

In my opinion, this error message is a bit cryptic and makes it difficult to figure out what the issue is. This PR adds explicit checks for missing command/options names/descriptions, and throws a more useful error message.

Status and versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • It changes the type of error that is thrown by SlashCommandBuilder::toJSON
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
    • If any dependents are making checks against the error thrown by SlashCommandBuilder::toJSON, this PR would break that

@wwselleck wwselleck requested a review from a team as a code owner April 28, 2023 05:57
@vercel
Copy link

vercel bot commented Apr 28, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
discord-js ⬜️ Ignored (Inspect) Apr 28, 2023 11:20pm
discord-js-guide ⬜️ Ignored (Inspect) Apr 28, 2023 11:20pm

@Jiralite Jiralite added this to the builders 1.7.0 milestone Apr 28, 2023
@wwselleck wwselleck force-pushed the improve-missing-command-parameters-error-message branch from 7fbef78 to f8fdd34 Compare April 28, 2023 23:20
@@ -35,14 +40,22 @@ export function validateMaxOptionsLength(options: unknown): asserts options is T
}

export function validateRequiredParameters(
name: string,
description: string,
name: string | undefined,
Copy link
Author

Choose a reason for hiding this comment

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

name and description are casted as non-undefined on SlashCommandBuilder, but I figure they should be treated as potentially undefined wherever they can be, since they can be undefined.

@codecov
Copy link

codecov bot commented Apr 29, 2023

Codecov Report

Merging #9468 (f8fdd34) into main (77191a2) will increase coverage by 0.02%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##             main    #9468      +/-   ##
==========================================
+ Coverage   59.29%   59.31%   +0.02%     
==========================================
  Files         223      223              
  Lines       14633    14646      +13     
  Branches     1258     1263       +5     
==========================================
+ Hits         8676     8688      +12     
- Misses       5917     5918       +1     
  Partials       40       40              
Flag Coverage Δ
builders 98.55% <93.33%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...lders/src/interactions/slashCommands/Assertions.ts 97.34% <93.33%> (-0.66%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines +8 to +12
export class MissingRequiredParameterError extends Error {
public constructor(missingParameter: string) {
super(`Required parameter "${missingParameter}" is missing`);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to create this (and make the function return non-shapeshift errors), you can use the MissingPropertyError error class from @sapphire/shapeshift, which is exported.

Using the aforementioned class:

import { MissingPropertyError } from '@sapphire/shapeshift';

throw new MissingPropertyError('name');

@Jiralite Jiralite removed this from the builders 1.8.0 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review in Progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants