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 support for fetching multiple guilds #5472

Merged
merged 12 commits into from May 29, 2021
Merged

feat: add support for fetching multiple guilds #5472

merged 12 commits into from May 29, 2021

Conversation

vaporoxx
Copy link
Contributor

@vaporoxx vaporoxx commented Apr 3, 2021

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

This PR adds support for GET /users/@me/guilds. They have a weird set of properties, so I made a separate class for them.

Status and versioning classification:

  • I know how to update typings and have done so
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

@vaporoxx
Copy link
Contributor Author

Since Guild#owner got removed with the latest commit, should I just add the missing properties to Guild instead of creating a new class?
cc @vladfrangu

@vladfrangu
Copy link
Member

Personally, I'd say no.. they still come from a different route altogether 👀

What do you think @iCrawl @kyranet?

@kyranet
Copy link
Member

kyranet commented May 7, 2021

Personally, I'd say no.. they still come from a different route altogether 👀

What do you think @iCrawl @kyranet?

Are there different properties in owner that are available to OAuth2's Guild but not in regular Guild? Is PartialGuild just a subset ot Guild?

If the answers are no and yes respectively, then we don't need a separate structure.

However, if they differ, generalizing them might throw away some potentially useful data, so an extra structure would be preferred.

@vaporoxx
Copy link
Contributor Author

vaporoxx commented May 7, 2021

OAuth2's guild has two additional properties (namely .owner and .permissions), but these don't exist on the regular guild object at all, so there are no conflicts between both classes

@kyranet
Copy link
Member

kyranet commented May 7, 2021

OAuth2's guild has two additional properties (namely .owner and .permissions), but these don't exist on the regular guild object at all, so there are no conflicts between both classes

In that case, a separate structure would be preferred so this data is readily available.

@vaporoxx
Copy link
Contributor Author

vaporoxx commented May 7, 2021

They could also be added to Guild as nullable properties 👀

Another thing I considered is that if we decide to keep them separate, maybe OAuth2Guild would be a better name?

@SpaceEEC
Copy link
Member

I'd prefer OAuth2Guild over PartialGuild, especially with our partials that name is just confusing.
I'd also prefer a separate class so consumers know what properties are actually (not) available without having to check upstream documentation or trial and error it out.

If we want to abstract this we could go for a BaseGuild which is inherited by Guild, OAuth2Guild(, and InviteGuild, if we want to go down that path).

@vaporoxx
Copy link
Contributor Author

vaporoxx commented May 10, 2021

Done, however the guild constructor is really messy so I'm not sure if I broke anything there

Edit: Looks like I somehow force-pushed all the new files away, will fix soon

@iCrawl
Copy link
Member

iCrawl commented May 28, 2021

This needs another rebase.

src/structures/BaseGuild.js Outdated Show resolved Hide resolved
Co-authored-by: SpaceEEC <spaceeec@yahoo.com>
@kyranet kyranet requested a review from vladfrangu May 29, 2021 16:41
@iCrawl iCrawl merged commit 48d6850 into discordjs:master May 29, 2021
@iCrawl iCrawl added this to the Version 13 milestone Jun 27, 2021
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

6 participants