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(*): document and support embeds field in message create endpoint #5792

Merged
merged 14 commits into from Jun 11, 2021

Conversation

castdrian
Copy link
Contributor

@castdrian castdrian commented Jun 9, 2021

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

The message create endpoint now supports up to 10 embeds just like the webhook endpoint, these changes reflect that addition within the typings and APIMessage#create function.

Status and versioning classification:

  • This PR changes the library's interface (methods or parameters added)
  • 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 includes breaking changes (methods removed or renamed, parameters moved or removed)

@castdrian castdrian marked this pull request as draft June 10, 2021 00:54
@ckohen
Copy link
Member

ckohen commented Jun 10, 2021

Upstream has been merged 🚀 changes are live.

@kyranet kyranet self-requested a review June 10, 2021 05:50
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.

Since Discord seems to plan the removal of the embed field (in favour of embeds), should we refactor the code and account for less overloads? I see this as a great opportunity to remove some conditional checks made for webhooks (and interactions).

As for the embed option, should we keep it as an alias for embeds: [embed]? Since { embed } is much nicer and shorter to write (and we already have an alias for content, so why not?).

Copy link
Contributor

@GoldenAngel2 GoldenAngel2 left a comment

Choose a reason for hiding this comment

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

src/structures/APIMessage.js#L147 needs to be updated aswell, so this works properly.

@castdrian
Copy link
Contributor Author

src/structures/APIMessage.js#L147 needs to be updated aswell, so this works properly.

I am aware, please refer to the initial comment in italics, I somewhat mistimed opening the PR yesterday

@castdrian castdrian changed the title feat(MessageOptions): document embeds field in typings feat(*): document and support embeds field message create endpoint Jun 10, 2021
@castdrian castdrian changed the title feat(*): document and support embeds field message create endpoint feat(*): document and support embeds field in message create endpoint Jun 10, 2021
@castdrian castdrian marked this pull request as ready for review June 10, 2021 11:52
@castdrian
Copy link
Contributor Author

Since Discord seems to plan the removal of the embed field (in favour of embeds), should we refactor the code and account for less overloads? I see this as a great opportunity to remove some conditional checks made for webhooks (and interactions).

As for the embed option, should we keep it as an alias for embeds: [embed]? Since { embed } is much nicer and shorter to write (and we already have an alias for content, so why not?).

Changes adressed, tested & rebased :)

@csuvajit
Copy link

csuvajit commented Jun 10, 2021

src/structures/APIMessage.js#L206 needs some changes here.

Copy link
Member

@ckohen ckohen left a comment

Choose a reason for hiding this comment

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

* @property {MessageEmbed|Object} [embed] An embed for the message
needs updating.

typings/index.d.ts Outdated Show resolved Hide resolved
@castdrian
Copy link
Contributor Author

TextBasedChannel.js#L73 needs updating.

Done, thank you

fix(TextChannel): JSDoc doesn't seem to like this, revert
@iCrawl iCrawl merged commit 99ff715 into discordjs:master Jun 11, 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.

None yet