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(ApplicationCommand): add #equals #6414

Merged
merged 7 commits into from Aug 30, 2021
Merged

Conversation

ckohen
Copy link
Member

@ckohen ckohen commented Aug 13, 2021

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:

  • 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)

Co-authored-by: Juruel Keanu Lorenzo <keanulorenzo32@gmail.com>
typings/index.d.ts Outdated Show resolved Hide resolved
Co-authored-by: Juruel Keanu Lorenzo <keanulorenzo32@gmail.com>
src/structures/ApplicationCommand.js Outdated Show resolved Hide resolved
src/structures/ApplicationCommand.js Outdated Show resolved Hide resolved
src/structures/ApplicationCommand.js Outdated Show resolved Hide resolved
src/structures/ApplicationCommand.js Outdated Show resolved Hide resolved
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>
Co-authored-by: Sugden <28943913+NotSugden@users.noreply.github.com>
Comment on lines +177 to +178
// TODO: remove ?? 0 on each when nullable
(command.options?.length ?? 0) !== (this.options?.length ?? 0) ||
Copy link
Contributor

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

Copy link
Member Author

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

Copy link
Contributor

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

src/structures/ApplicationCommand.js Outdated Show resolved Hide resolved
src/structures/ApplicationCommand.js Outdated Show resolved Hide resolved
Copy link
Member

@kyranet kyranet left a 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.

@ImRodry
Copy link
Contributor

ImRodry commented Aug 29, 2021

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.
Also why do you still have TODO comments?

@ckohen
Copy link
Member Author

ckohen commented Aug 29, 2021

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.

@ImRodry
Copy link
Contributor

ImRodry commented Aug 29, 2021

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.
As for the TODO comments, why are they for v14?

@iCrawl iCrawl merged commit 581921f into discordjs:main Aug 30, 2021
@ckohen ckohen deleted the app-command-equal branch August 31, 2021 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants