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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Managers): new ApplicationCommandPermissionsManager #5897

Merged
merged 7 commits into from Jun 29, 2021

Conversation

ckohen
Copy link
Member

@ckohen ckohen commented Jun 21, 2021

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

Replaces and closes #5695 as the PR seems to be abandoned. This takes a good chunk of the code from that PR, but moves existing code and new code into a new manager while addressing requested changes.

Benefits of this approach:
Each command has its own way to access all of its permissions properties alongside the guild and global versions
Every function takes an object, making the somewhat confusing required vs optional parameters a little easier to understand.
Typescript users hopefully know exactly what they need to use the manager due to some hefty overloads and type passing.
(I did a lot of things that I don't know whether will work or not, but they should 馃...I hope )

鈿狅笍 This is untested as of now, testing in progress.

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)
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

src/managers/ApplicationCommandPermissionsManager.js Outdated Show resolved Hide resolved
src/managers/ApplicationCommandPermissionsManager.js Outdated Show resolved Hide resolved
@ckohen ckohen marked this pull request as ready for review June 21, 2021 20:03
src/managers/ApplicationCommandPermissionsManager.js Outdated Show resolved Hide resolved
src/managers/ApplicationCommandPermissionsManager.js Outdated Show resolved Hide resolved
src/managers/ApplicationCommandPermissionsManager.js Outdated Show resolved Hide resolved
src/structures/ApplicationCommand.js Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
@iCrawl
Copy link
Member

iCrawl commented Jun 23, 2021

This needs a rebase.

@iCrawl iCrawl dismissed stale reviews from vladfrangu and kyranet June 24, 2021 08:14

Stale

src/util/Constants.js Outdated Show resolved Hide resolved
@iCrawl
Copy link
Member

iCrawl commented Jun 24, 2021

This needs a rebase.

src/util/Constants.js Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
ckohen and others added 6 commits June 27, 2021 01:12
Co-Authored-By: SpaceEEC <24881032+SpaceEEC@users.noreply.github.com>
Co-authored-by: Yoshida Tomio <mail@tomio.codes>
Co-authored-by: Antonio Rom谩n <kyradiscord@gmail.com>
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>
typings/index.d.ts Outdated Show resolved Hide resolved
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>
@ckohen
Copy link
Member Author

ckohen commented Jun 27, 2021

Sorry about the commit msg on the last commit, did it from mobile. If we hit another rebase I'll fix it

@iCrawl iCrawl added this to the Version 13 milestone Jun 27, 2021
@iCrawl iCrawl merged commit 6264c60 into discordjs:master Jun 29, 2021
@ckohen ckohen deleted the appcommand-perm-manager branch June 29, 2021 21:53
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