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(StageChannel): add createStageInstance method & use better naming convention #5951

Merged
merged 4 commits into from
Jul 1, 2021

Conversation

iShibi
Copy link
Contributor

@iShibi iShibi commented Jun 28, 2021

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

This PR adds a new createStageInstance method on StageChannel that lets the user create a stage instance associated to it. It also refactors the options typedef to keep it consistent with rest of the lib:

  1. CreateStageInstanceOptions is now StageInstanceCreateOptions
  2. The channel field is a seperate param now instead of being inside the options object

⚠️ As pointed out by @SpaceEEC, the term "instance" makes it sound like it is an instance of a Stage Channel which it isn't. This PR removes this ambiguity by using the complete term i.e. "Stage Instance" in all places.

Status and versioning classification:

  • Code changes have been tested against the Discord API
  • I know how to update typings and have done so
  • This PR changes the library's interface
  • This PR includes breaking changes

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.

I don't agree with the breaking change as the object still has a required property (topic).

If it's because StageChannel#createStageInstance (probably should be createInstance), you can do create({ ...options, channel: this.id }).

@iShibi
Copy link
Contributor Author

iShibi commented Jun 29, 2021

It will be the same situation like MessageOptions again, where messageReference was getting set to this.id and hence ReplyMessageOptions was created. Unlike there, where we couldn't make messageReference a seperate param, it is much more intuitive to have channel as a first param to stageInstances#create here.

createInstance sounds good tho, will rename it.

@iCrawl iCrawl requested a review from kyranet June 29, 2021 10:54
@iCrawl iCrawl added this to the Version 13 milestone Jun 29, 2021
src/structures/StageChannel.js Outdated Show resolved Hide resolved
src/managers/StageInstanceManager.js Outdated Show resolved Hide resolved
@iShibi iShibi changed the title feat(StageChannel): add method to create instance feat(StageChannel): add createStageInstance method & use better naming convention Jun 30, 2021
@iShibi
Copy link
Contributor Author

iShibi commented Jun 30, 2021

Refactored some stuff to reflect the new naming convention for things related to stage instances. Added more information covering this change in the description above and changed the title.

"instance of stage channel" What was I thinking pepehands

src/managers/StageInstanceManager.js Show resolved Hide resolved
@iCrawl iCrawl merged commit 71fb33a into discordjs:master Jul 1, 2021
@iShibi iShibi deleted the refactor-si branch August 14, 2021 12:02
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.

5 participants