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(Threads): API v9, add rest support for Threads #31

Open
wants to merge 7 commits into
base: beta
Choose a base branch
from

Conversation

SiroTM
Copy link
Collaborator

@SiroTM SiroTM commented Jun 6, 2021

This PR

  • Adds thread related events in ws\constants.ts.
  • Adds thread related properties in structures\Guild.ts and src\util\Constants.ts.
  • Adds thread related methods in src\Client.ts and rest\Endpoints.ts.
  • Changes API v8 to API v9 in rest\RESTClient.ts

Notes:

@bsian03
Copy link
Member

bsian03 commented Jun 6, 2021

There have been a couple of changes since that PR as well, did you go through relevant doc commits after that as well?

@SiroTM
Copy link
Collaborator Author

SiroTM commented Jun 6, 2021

There have been a couple of changes since that PR as well, did you go through relevant doc commits after that as well?

I took into consideration the link I put in the PR description and the discord documentation which should be updated
I don't think I missed things, but tell me if I did

Copy link
Member

@bsian03 bsian03 left a comment

Choose a reason for hiding this comment

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

We're not using src folder. Also endpoint descriptions should just be how they are named on Discord

Comment on lines 386 to 396
public static THREAD(channelID: string) {
return `/channels/${channelID}/thread-members/@me`;
}
/**
* `/channels/{channel.id}/thread-members/{user.id}`
* - PUT - Adds another member to a thread
* - DELETE - Removes another member from a thread
*/
public static THREAD_MEMBER(channelID: string, userID: string) {
return `/channels/${channelID}/thread-members/${userID}`;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm tempted to say combine these, cc @Khaaz?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure, what did we do for other similar endpoints?
I'm not against specific /@me endpoint, although I do agree it's legitimate to combine.

rest/Endpoints.ts Outdated Show resolved Hide resolved
rest/Endpoints.ts Show resolved Hide resolved
Comment on lines 386 to 396
public static THREAD(channelID: string) {
return `/channels/${channelID}/thread-members/@me`;
}
/**
* `/channels/{channel.id}/thread-members/{user.id}`
* - PUT - Adds another member to a thread
* - DELETE - Removes another member from a thread
*/
public static THREAD_MEMBER(channelID: string, userID: string) {
return `/channels/${channelID}/thread-members/${userID}`;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure, what did we do for other similar endpoints?
I'm not against specific /@me endpoint, although I do agree it's legitimate to combine.

Copy link
Collaborator

@Khaaz Khaaz left a comment

Choose a reason for hiding this comment

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

other than that seems good so far?

Copy link
Member

@bsian03 bsian03 left a comment

Choose a reason for hiding this comment

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

Again, endpoint descriptions should be the endpoint name on Discord, i.e. POST START_THREAD_WITH_MESSAGE be Start Thread with Message

@Khaaz Khaaz changed the title feat(Threads) feat(Threads): API v9, add rest support for Threads Jun 12, 2021
@Khaaz
Copy link
Collaborator

Khaaz commented Jun 12, 2021

Apart from the naming and Bsian point, this PR seems globally ready I think. Do we have anything else to change for API v9?
I also want to highlight this is rest only for threads (which is fine for this PR).

Copy link
Member

@bsian03 bsian03 left a comment

Choose a reason for hiding this comment

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

Can you revert all the src/ changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants