Skip to content
This repository has been archived by the owner on Jan 8, 2022. It is now read-only.

feat: create Embed builder #11

Merged
merged 36 commits into from
Aug 24, 2021
Merged

feat: create Embed builder #11

merged 36 commits into from
Aug 24, 2021

Conversation

Fyko
Copy link
Contributor

@Fyko Fyko commented Jul 12, 2021

Please describe the changes this PR makes and why it should be merged:
This PR creates an Embed builder. Closes #10

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)

@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2021

Codecov Report

Merging #11 (01bbcb8) into main (2aecbe4) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #11   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        18    +3     
  Lines          144       227   +83     
  Branches        21        33   +12     
=========================================
+ Hits           144       227   +83     
Impacted Files Coverage Δ
src/messages/embed/Assertions.ts 100.00% <100.00%> (ø)
src/messages/embed/Embed.ts 100.00% <100.00%> (ø)
.../interactions/slashCommands/SlashCommandBuilder.ts 100.00% <0.00%> (ø)
...nteractions/slashCommands/mixins/CommandOptions.ts 100.00% <0.00%> (ø)
...s/slashCommands/mixins/CommandOptionWithChoices.ts 100.00% <0.00%> (ø)
src/interactions/slashCommands/options/number.ts 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2aecbe4...01bbcb8. Read the comment docs.

Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

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

Overall this feels more like a copy-paste from d.js (fair), without any input validation or such. Should probably get some of that rolling

src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
Copy link

@memikri memikri left a comment

Choose a reason for hiding this comment

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

Embed methods should return this instead of Embed to allow for chaining from extending classes.

src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
@DTrombett
Copy link
Contributor

I think this should check if all the properties are valid before setting them, like check if the title provided in setTitle is shorter than 256 characters, or if the fields are less than 25...

@Fyko
Copy link
Contributor Author

Fyko commented Jul 17, 2021

I think this should check if all the properties are valid before setting them, like check if the title provided in setTitle is shorter than 256 characters, or if the fields are less than 25...

That's what Vlad suggested in #11 (comment) but we're waiting on a response from the big three

@vladfrangu
Copy link
Member

vladfrangu commented Jul 18, 2021

I think this should check if all the properties are valid before setting them, like check if the title provided in setTitle is shorter than 256 characters, or if the fields are less than 25...

That's what Vlad suggested in #11 (comment) but we're waiting on a response from the big three

I pinged the other two to reach a consensus about passing in data in the constructor, not validating builder methods inputs :D

Also this needs a rebase

src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts 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.

Otherwise LGTM.

src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
@vladfrangu
Copy link
Member

  • Passing data in the constructor stays!
  • Validation with ow should occur in the functions (setTitle should assert that it's a string of maxlength, etc)
  • Constructors shouldn't call setter functions for validating data

Any questions, hit me up @Fyko!

@Fyko Fyko requested a review from vladfrangu July 20, 2021 22:24
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed.ts Outdated Show resolved Hide resolved
src/messages/embed/Embed.ts Outdated Show resolved Hide resolved
src/messages/embed/Embed.ts Outdated Show resolved Hide resolved
src/messages/embed/Embed.ts Outdated Show resolved Hide resolved
@iCrawl iCrawl requested a review from kyranet July 31, 2021 08:58
@iCrawl iCrawl dismissed stale reviews from kyranet and vladfrangu July 31, 2021 08:58

Stale

src/messages/embed/Embed.ts Outdated Show resolved Hide resolved
src/messages/embed/Embed.ts Show resolved Hide resolved
src/messages/embed/Embed.ts Show resolved Hide resolved
Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

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

You may leave your complaints in my DMs 😬

src/messages/embed/Assertions.ts Outdated Show resolved Hide resolved
src/messages/embed/Assertions.ts Outdated Show resolved Hide resolved
src/messages/embed/Embed.ts Outdated Show resolved Hide resolved
src/messages/embed/Embed.ts Outdated Show resolved Hide resolved
src/messages/embed/Embed.ts Outdated Show resolved Hide resolved
src/messages/embed/Embed.ts Outdated Show resolved Hide resolved
src/messages/embed/Embed.ts Show resolved Hide resolved
src/messages/embed/Embed.ts Outdated Show resolved Hide resolved
src/messages/embed/Embed.ts Outdated Show resolved Hide resolved
src/messages/embed/Embed.ts Outdated Show resolved Hide resolved
@Fyko Fyko requested a review from vladfrangu August 6, 2021 21:28
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/messages/embed/Embed.ts Outdated Show resolved Hide resolved
src/messages/embed/Embed.ts Outdated Show resolved Hide resolved
Fyko and others added 3 commits August 21, 2021 13:41
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>
@iCrawl iCrawl merged commit eb942a4 into discordjs:main Aug 24, 2021
Khasms pushed a commit to Khasms/builders that referenced this pull request Sep 21, 2021
Co-authored-by: DTrombett <73136330+DTrombett@users.noreply.github.com>
Co-authored-by: Antonio Román <kyradiscord@gmail.com>
Co-authored-by: Sugden <28943913+NotSugden@users.noreply.github.com>
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add MessageEmbed
9 participants