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: add components to /builders #7195

Merged
merged 8 commits into from Jan 12, 2022

Conversation

suneettipirneni
Copy link
Member

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

Adds all current component types to /builders.

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)

@iCrawl iCrawl removed the chore label Jan 7, 2022
@suneettipirneni suneettipirneni changed the title feat: add components to builders feat: add components to /builders Jan 7, 2022
@iCrawl iCrawl added this to the builders v1 milestone Jan 7, 2022
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.

Looks fine to me. Can you not implement something similar to application command options (relating to the toJSON() being mostly {...this}?

@suneettipirneni
Copy link
Member Author

Looks fine to me. Can you not implement something similar to application command options (relating to the toJSON() being mostly {...this}?

I can, I just didn't know if handling snake_case like this:

{
	...this
    snake_case_prop: this.camelCaseProp,
}

was preferred or not

@vladfrangu
Copy link
Member

It's what I did for builders (and subsequently forgot to do consistently), but CC @discordjs/the-big-4 about this

@suneettipirneni
Copy link
Member Author

It's what I did for builders (and subsequently forgot to do consistently), but CC @discordjs/the-big-4 about this

Also I just realized that if there are snake_case keys, and you do ...this you'll have the leftover camelcase key in the json. So that'll also have to be manually deleted off the object.

@vladfrangu
Copy link
Member

It's what I did for builders (and subsequently forgot to do consistently), but CC @discordjs/the-big-4 about this

Also I just realized that if there are snake_case keys, and you do ...this you'll have the leftover camelcase key in the json. So that'll also have to be manually deleted off the object.

Just replace all camelCase props on the class with snake_case 🦐

@iCrawl
Copy link
Member

iCrawl commented Jan 8, 2022

This needs a rebase.

@suneettipirneni suneettipirneni force-pushed the feat/builders/components branch 2 times, most recently from 5a309aa to c7c3b74 Compare January 8, 2022 13:33
Copy link
Member

@SpaceEEC SpaceEEC left a comment

Choose a reason for hiding this comment

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

Are the changes to the yarn.lock file intentional? 👀

packages/builders/src/components/Assertions.ts Outdated Show resolved Hide resolved
packages/builders/src/components/Assertions.ts Outdated Show resolved Hide resolved
@suneettipirneni
Copy link
Member Author

Note: CI failing because of voice tests

@iCrawl iCrawl merged commit 2bb40fd into discordjs:main Jan 12, 2022
@suneettipirneni suneettipirneni deleted the feat/builders/components branch February 7, 2022 16:25
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.

None yet

5 participants