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

Add ID permission handlers #7

Merged
merged 5 commits into from
Jul 19, 2021
Merged

Add ID permission handlers #7

merged 5 commits into from
Jul 19, 2021

Conversation

suneettipirneni
Copy link
Member

There are new helpers for ID-based checking:

const command: Command = {
  ...
  permissions: checkAll(
    RolePermissions.allIDs('855841786869841933', '855841786857127942'),
    ChannelPermissions.inIDs('855841788115288107'),
  )
  ...
};

@suneettipirneni suneettipirneni requested a review from rob-3 July 18, 2021 04:56
@suneettipirneni suneettipirneni self-assigned this Jul 18, 2021
@rob-3
Copy link
Contributor

rob-3 commented Jul 18, 2021

Looks good! Two suggestions (lmk what you think):

  1. Flatten the namespace and rename functions. I think this will make this a bit less clunky for use with helpers like checkAll; for example:
checkAll(RolesPermissions.oneName(Roles.president), ChannelPermissions.inChannels(Channels.exec_board))

vs

checkAll(inRoles(Roles.president), inChannels(Channels.exec_board))

Some suggestions for renaming:

  • RolePermissions.allNames => allRoleNames
  • RolePermissions.oneName => inRoleNames (this keeps the wording the same as the ChannelPermissions)
  • RolePermissions.allIDs => allRoles (this will encourage people to use the Snowflake way by default and get all the benefits we've discussed)
  • RolePermissions.oneID => inRoles
  • ChannelPermissions.inNames => inChannelNames
  • ChannelPermissions.inIDs => inChannels
  • CategoryPermissions.inNames => inCategoryNames
  • CategoryPermissions.inIDs => inCategories
  1. (maybe out of scope for this PR) We have a checkAll() helper but not a "checkOne()" or similar thing to express logic like: "either be in #general OR be a director" to use this command

@rob-3
Copy link
Contributor

rob-3 commented Jul 18, 2021

Ok new commit is good. Do you prefer oneRoleName over inRoleNames? It bothers me a little how the channel and role versions of the same functions have different prefixes.

@suneettipirneni suneettipirneni merged commit dc197e0 into main Jul 19, 2021
@suneettipirneni suneettipirneni deleted the dev/id-checking branch July 19, 2021 01:14
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.

2 participants