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

refactor(MessageOptions): move replyTo to reply#messageReference and add failIfNotExists #5298

Merged
merged 8 commits into from
May 10, 2021

Conversation

ckohen
Copy link
Member

@ckohen ckohen commented Feb 6, 2021

Please describe the changes this PR makes and why it should be merged:

Add support for new fail_if_not_exists parameter in message_reference
ref: discord/discord-api-docs#2572

This also changes the replyTo parameter to be more in line with the API

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)

@iShibi
Copy link
Contributor

iShibi commented Feb 6, 2021

Nice 👍, I was just thinking that shouldn't the field name emphasize more on the error part. The API calls it fail_if_not_exists, which makes it clear that something will fail. Even though our description of this field states that it will error, the name doesn't sound like an error will occur.

@iShibi
Copy link
Contributor

iShibi commented Feb 6, 2021

I would suggest the name errorIfDeleted for this field. The reason being that:

  1. It emphasizes the error part and conveys the meaning of the field better.
  2. It will be in line with the API. Right now we are flipping the booleans and because of the field name, our default for this field is opposite of the API's default. The errorIfDeleted can be documented to be true by default just like fail_if_not_exists.

@ckohen
Copy link
Member Author

ckohen commented Feb 6, 2021

My only issue with errorIfDeleted is that it doesn't indicate it only applies to replies and is a confusing name when not in that context. I will consider this though

@vaporoxx
Copy link
Contributor

vaporoxx commented Feb 6, 2021

deleted shouldn't be included in the name at all since passing an invalid id would also have different behavior depending on the parameter. I think that errorOnInvalidReply or similar would maybe be a better name here

@almostSouji
Copy link
Member

So far we've always decided to stick with the API names for options, I'd personally vote for this specific one to not become an exception.

@ckohen
Copy link
Member Author

ckohen commented Feb 8, 2021

Hmm, I would like to stick with API names as there is clearly a lot of thought in creating these names, but that poses the problem that the library doesn't expose message_reference whatsoever. This means that MessageOpitons#failIfNotExists has no indication that it is reply specific other than the jsdoc string.

Maybe this needs to wait until #5296 lands and have that name? Even then I still don't like that it has no indication of being part of message_reference.

@monbrey
Copy link
Member

monbrey commented Feb 8, 2021

Hmm, I would like to stick with API names as there is clearly a lot of thought in creating these names, but that poses the problem that the library doesn't expose message_reference whatsoever. This means that MessageOpitons#failIfNotExists has no indication that it is reply specific other than the jsdoc string.

Maybe this needs to wait until #5296 lands and have that name? Even then I still don't like that it has no indication of being part of message_reference.

Replies haven't been released in a stable release yet. If you want to rename MessageOptions#replyTo to messageReference I don't see the issue with that.

@ckohen
Copy link
Member Author

ckohen commented Feb 8, 2021

Yep, Souji and I talked about it in the discord, and this is going to end up being a refactor + add

@ckohen ckohen changed the title feat(MessageOptions): add support for fail_if_not_exists in replies refactor(MessageOptions): move replyTo to reply#messageReference and add failIfNotExists Feb 8, 2021
typings/index.d.ts Outdated Show resolved Hide resolved
src/structures/interfaces/TextBasedChannel.js Outdated Show resolved Hide resolved
Co-authored-by: SpaceEEC <spaceeec@yahoo.com>
iShibi added a commit to iShibi/discord.js that referenced this pull request Feb 12, 2021
Co-authored-by: Shubham Parihar <shubhamparihar391@gmail.com>
@iCrawl iCrawl requested a review from kyranet April 3, 2021 13:14
@kyranet kyranet requested a review from iCrawl April 3, 2021 16:28
@iShibi
Copy link
Contributor

iShibi commented Apr 12, 2021

⚠️ Typings for WebhookMessageOptions will need another update (see #5476) once this PR merges.

@iCrawl iCrawl merged commit 1ecda83 into discordjs:master May 10, 2021
@ckohen ckohen deleted the add-reply-fail branch May 10, 2021 10:32
iShibi added a commit to iShibi/discord.js that referenced this pull request May 10, 2021
@iCrawl iCrawl added this to the Version 13 milestone Jun 27, 2021
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.

Message.reply causes an error if the author's message was deleted
9 participants