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

docs: cleanup MessageCreateOptions and MessageReplyOptions #9283

Merged
merged 9 commits into from Apr 14, 2023

Conversation

BoogeyMan24
Copy link
Contributor

@BoogeyMan24 BoogeyMan24 commented Mar 27, 2023

Please describe the changes this PR makes and why it should be merged:
Changed BaseMessageCreateOptions to BaseMessageOptions and added tts, nonce, stickers, and flags as well. All near line 822 in discordjs/src/structures/Messages.js.

Hopefully this fixes #9209.

Edit: As suggested by Jiralite and for deduplication purposes, BaseMessageCreateOptions has been created to contain the the shared data of MessageCreateOptions and MessageReplyOptions. MessageCreateOptions and MessageReplyOptions have been updated to extend BaseMessageCreateOptions and have their respective special properties added reply, and failIfNotExists.

I probably may have missed something, so if I did just send a comment and I will change it.

Status and versioning classification:
There are no code changes
This PR only includes non-code changes, like changes to documentation, README, etc.

@vercel
Copy link

vercel bot commented Mar 27, 2023

@BoogeyMan24 is attempting to deploy a commit to the discordjs Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Mar 27, 2023

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

Name Status Preview Comments Updated (UTC)
discord-js ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 14, 2023 9:49pm
discord-js-guide ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 14, 2023 9:49pm

@github-actions
Copy link

github-actions bot commented Mar 27, 2023

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 87
🟢 Accessibility 100
🟢 Best practices 100
🟢 SEO 100
🟠 PWA 70

Lighthouse ran on https://discord-js-git-fork-boogeyman24-main-discordjs.vercel.app/

@jaw0r3k
Copy link
Contributor

jaw0r3k commented Mar 27, 2023

But it already has pr?
Also why have you deleted stickers?

@BoogeyMan24
Copy link
Contributor Author

I deleted stickers because MessageCreateOptions already has stickers.

Copy link
Member

@Jiralite Jiralite left a comment

Choose a reason for hiding this comment

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

Extending MessageCreateOptions isn't correct. It contains irrelevant properties such as reply, etc.

@BoogeyMan24
Copy link
Contributor Author

BoogeyMan24 commented Mar 27, 2023

I will just extend BaseMessageOptions and add everything that MessageCreateOptions has except reply.

@Jiralite
Copy link
Member

Jiralite commented Apr 1, 2023

For purposes of deduplication, I would:

  1. Create BaseMessageCreateOptions which has everything MessageCreateOptions has right now except reply.
  2. Have MessageCreateOptions extend BaseMessageCreateOptions and add reply.
  3. Create MessageReplyOptions and have it extend BaseMessageCreateOptions and add failIfNotExists.

@BoogeyMan24
Copy link
Contributor Author

BoogeyMan24 commented Apr 10, 2023

I added the changes for deduplication as you suggested.

@Jiralite Jiralite added this to the discord.js 14.10 milestone Apr 10, 2023
@Jiralite Jiralite mentioned this pull request Apr 12, 2023
@vladfrangu vladfrangu changed the title Fixed MessageReplyOptions chore(docs): cleanup MessageReplyOptions Apr 13, 2023
@vladfrangu vladfrangu changed the title chore(docs): cleanup MessageReplyOptions chore(docs): cleanup MessageCreateOptions and MessageReplyOptions Apr 13, 2023
@vladfrangu vladfrangu changed the title chore(docs): cleanup MessageCreateOptions and MessageReplyOptions docs: cleanup MessageCreateOptions and MessageReplyOptions Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

MessageReplyOptions references non-existent type definition BaseMessageCreateOptions
6 participants