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(ApplicationCommand): add #equals #6414
Conversation
Co-authored-by: Juruel Keanu Lorenzo <keanulorenzo32@gmail.com>
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com> Co-authored-by: Sugden <28943913+NotSugden@users.noreply.github.com>
// TODO: remove ?? 0 on each when nullable | ||
(command.options?.length ?? 0) !== (this.options?.length ?? 0) || |
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.
You don't actually need the ?? 0
because you'd be comparing undefined to undefined if neither one of the commands have options, which is still valid and produces the same output
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.
this.options
always exists currently, the ?.
on the RHS is misleading. I could remove the one on the RHS only but this is accounting for the (yes unlikely) case that someone changed that property in runtime
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.
They can also provide an object that they construct in which case it won't have options so it's good to keep both ?. there, just saying that because of your TODO comment, since you can easily remove those
Co-authored-by: Rodry <38259440+ImRodry@users.noreply.github.com>
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.
We really need unit tests for this, but looks good to me.
I strongly disagree with the need for an option here and specially it being set to false by default. Option order is something that will change how Discord handles your commands so if you really want that option it should default to true. The best in my opinion would be to default to true and remove that option. |
I have TODO comments because they're TODO for v14, not for now. As for your disagreement. No, order doesn't change how discord handles your commands, it changes how it is stored on the API. As I've already said, the client does not strictly respect this order and the info blocks I added for those parameters clearly indicates the thought process for that. |
The client may not strictly respect that order, but it does to some extent. So if you change the order of an option or a subcommand even, the client will display it differently, which is why I think it should at least default to true. |
Please describe the changes this PR makes and why it should be merged:
Adds
ApplicationCommand#equals
since checking the equivalence of application commands is a non-trivial task. Especially when handling the instantiated class and raw data.Oh, and also because @IanMitchell asked for it
Status and versioning classification: