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(GuildChannel): make createOverwrite and updateOverwrite not dependent on cache #5489

Merged
merged 10 commits into from
May 11, 2021

Conversation

iShibi
Copy link
Contributor

@iShibi iShibi commented Apr 6, 2021

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

The above two methods need userOrRole to be in cache, so that they can be resolved in order to get the type of overwrite. This in some edge cases where the provided userOrRole is id of an uncached structure will throw an error. This PR adds a new optional parameter type for these two methods. Which can be used to manually set the type of the overwrite, making the methods not rely on the cache just to get the type.

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • This PR changes the library's interface (methods or parameters added)
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

src/structures/GuildChannel.js Outdated Show resolved Hide resolved
Copy link
Contributor

@NotSugden NotSugden left a comment

Choose a reason for hiding this comment

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

typings need updating too

src/structures/GuildChannel.js Outdated Show resolved Hide resolved
@ckohen
Copy link
Member

ckohen commented Apr 6, 2021

I'd just like to point out that this fixes 2 of 8 methods that have this issue, most of the other ones take a role which is more likely to be cached, but there are still edge cases. I really like the way this was handled, not sure if this PR would be a good place to make changes to the other methods as well.

For reference, you can see these other methods in #5242

src/structures/GuildChannel.js 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.

Just one change to make CI pass

src/structures/GuildChannel.js Outdated Show resolved Hide resolved
src/structures/GuildChannel.js Outdated Show resolved Hide resolved
src/structures/GuildChannel.js Outdated Show resolved Hide resolved
@iCrawl iCrawl requested a review from kyranet April 30, 2021 10:29
@iCrawl iCrawl requested a review from vladfrangu April 30, 2021 10:30
@iCrawl iCrawl dismissed stale reviews from vladfrangu and SpaceEEC April 30, 2021 10:30

stale

@vladfrangu
Copy link
Member

This PR clashes with #5318 (if the mentioned PR gets merged first, this will need a full rebase)... @iCrawl how should we handle this?

@iCrawl
Copy link
Member

iCrawl commented May 6, 2021

Depends on what makes more sense to merge. Does a whole new manager really make sense here, except to abstract things more thoroughly?

src/structures/GuildChannel.js Outdated Show resolved Hide resolved
src/structures/GuildChannel.js Outdated Show resolved Hide resolved
@iCrawl iCrawl merged commit 58763b0 into discordjs:master May 11, 2021
@iShibi iShibi deleted the feat-overwrite branch May 14, 2021 09:30
@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.

7 participants