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

Remove RichEmbed in favour of MessageEmbed #1584

Merged
merged 3 commits into from
Jul 3, 2017

Conversation

Drahcirius
Copy link
Member

@Drahcirius Drahcirius commented Jun 11, 2017

From #1529, MessageEmbeds are now identical to RichEmbeds (with a few differences).
There's no further reason in having separate MessageEmbed and RichEmbed classes, as they're similar enough that you could require MessageEmbed inside of RichEmbed and export it to get identical behaviour.

This simplifies internal usage as well. Typechecking #1451 for RichEmbed and MessageEmbed could be simplified to just MessageEmbed.

In this commit:
<MessageEmbed>._apiTransform() private method added so that internal variables can continue using camel case.
<MessageEmbed>.color getter returns null if colour is not set instead of throwing an error.
<MessageEmbed>.createdAt getter returns null if timestamp is unset/invalid.
<RichEmbed>.addBlankField added to MessageEmbed.

Most properties that used to to be set to null when missing now default to undefined, to be more consistent with current RichEmbed behaviour.

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

name: data.provider.name,
url: data.provider.url,
} : null;
this.provder = data.provider;
Copy link
Member

Choose a reason for hiding this comment

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

Nice typo?

Choose a reason for hiding this comment

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

There are unit tests, right? Won't they catch it?

@devsnek
Copy link
Member

devsnek commented Jun 13, 2017

can we just get rid of addBlankField

* @param {boolean} [inline=false] Set the field to display inline
* @returns {MessageEmbed} This embed
*/
addBlankField(inline = false) {
Copy link
Member

Choose a reason for hiding this comment

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

delet

Copy link
Member

Choose a reason for hiding this comment

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

No.

Copy link
Member

Choose a reason for hiding this comment

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

Why? Its such a useful small thing! Ain't nobody got time to get the unicode code for the 0-width space

@iCrawl iCrawl merged commit b1d9084 into discordjs:master Jul 3, 2017
@Drahcirius Drahcirius deleted the richembed-remove branch July 4, 2017 00:15
Ratismal pushed a commit to Ratismal/discord.js that referenced this pull request Jul 5, 2017
* remove RichEmbed in favour of MessageEmbed

* fix provider typo
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

7 participants