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

Permissions for(role) #1421

Closed
wants to merge 13 commits into from
Closed

Permissions for(role) #1421

wants to merge 13 commits into from

Conversation

bdistin
Copy link
Contributor

@bdistin bdistin commented Apr 28, 2017

Please describe the changes this PR makes and why it should be merged:
Allows role resolveables to be passed to permissionsFor() to check the resolved permissions for a given role after channel overwrites. (also adds a resolveRole method to the client data resolver, and cleans up a second instance where duplicate code existed)

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

@xDdude
Copy link
Contributor

xDdude commented Apr 28, 2017

iirc there is no typedef for RoleResolvable. There are also other instances (GuildMember role stuff) where the resolveRole method could be used for consistency. maybe that doesn't fit with this pr...

@devsnek
Copy link
Member

devsnek commented Apr 29, 2017

a shitload of code is duplicated in those the if/else can you reuse some code plz ❤️

@bdistin
Copy link
Contributor Author

bdistin commented Apr 30, 2017

Perhaps I am missing what you are seeing? But I see no code that can be reused... Given the way permissions must be applied to resolve correctly that is.

@rei2hu
Copy link
Contributor

rei2hu commented Apr 30, 2017

Perhaps I am missing what you are seeing? But I see no code that can be reused... Given the way permissions must be applied to resolve correctly that is.

I think there is always an everyone overwrite and assuming that's true, one way to do it is the following:

  1. Make an array with guild id for everyone overwrite
  2. If role is provided, push role id to array. If member is provided, push member id and role ids to array.
  3. Iterate through array and do the bitwise operations.

This might be a little shorter but maybe a little more inefficient.

@bdistin
Copy link
Contributor Author

bdistin commented Apr 30, 2017

That is specifically what you cannot do. Permissions must be applied in a specific manner according to https://support.discordapp.com/hc/en-us/articles/206141927-How-is-the-permission-hierarchy-structured- That was how permissionsFor was coded before an earlier pr of mine, and it caused incorrect permission resolution in several edge cases.

@bdistin
Copy link
Contributor Author

bdistin commented May 7, 2017


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

5 participants