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(Role): role tags #4628

Merged
merged 24 commits into from
Dec 14, 2020
Merged

feat(Role): role tags #4628

merged 24 commits into from
Dec 14, 2020

Conversation

almostSouji
Copy link
Member

@almostSouji almostSouji commented Jul 11, 2020

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

role data now includes tags, indicating if the role is the boost role (premium subscription in discord lingo), belongs to an integration or bot. This feature request aims to implement this new feature into discord.js while also introducing some useful getters

Changes breakdown:

  • Role#tags introduce the new API data into a #tags property on Role
  • Role#isMyRole *
  • Role#isPremiumSubscriberRole *
  • GuildMemberRoleManager#premiumSubscriberRole
  • GuildMemberRoleManager#botRole
  • RoleManager#premiumSubscriberRole
  • RoleManager#botRoleFor *
  • RoleManager#myRole *
  • (+) Integration#Roles (external PR) (see consideration 9.)
  • typings

* See pruning considerations below

The feature part dealing with integrations is only minimal and does not need to introduce new access or getters due to consideration 8. see consideration 9.

Considerations
  1. Naming: we use #premiumSince #premiumSubscriberCount for other boosting features. The API data key is premium_subscriber, so the camel-case version would be #premiumSubscriber, which however sounds more like a GuildMember than a Role instance, so the suffix -Role is added
  2. Naming: #myRole and #isMyRole after Guild#me
  3. Implementation: GuildMemberRolemanager#myRole could be argued as useful to find the bots role from Guild#me
  4. Implementation: Reflect the in operator along with this rather cumbersome check and assignment are used to only create the keys on the #tags object that are actually represented in the role data payload, otherwise we'd end up with some nasty undefined or null values.
  5. API: the value for the premium_subscriber key is null as per API spec, transformed to true here for convenience
  6. Implementation: I was thinking about GuildMember#isPremiumSubscriber, but since it is tightly knit to the role, figured it should be moved to the role manager instead. I then figured that additional utility could come from returning the member instead of the boolean setup of is* or has*. Note: This is not equivalent to GuildMember#guild#roles#premiumSubscriberRole as it can be used to check for said role on this specific member!
  7. Naming: RoleManager#botRoleFor akin to #permissionsFor and #botRole
  8. Implementation: Integration identifiers are more of a nieche part here and do not need new access as Integration#rolealready exists (see 9.)
  9. Implementation: Integrations can have multiple roles (level roles for twitch, for example) thusly Integration#roles has been introduced.
API pruning considerations

This API has become very convoluted, i don't think this is necessary or exactly beneficial and will trim it back with the following considerations:

  • Role#tags ✔ fundamental
  • Role#isMyRole ❌ was only used internally, feels clunky
  • Role#isPremiumSubscriberRole ❌ was only used internally, feels clunky
  • GuildMemberRoleManager#premiumSubscriberRole ✔ permission check
  • GuildMemberRoleManager#botRole ✔ easy access for a certain bots role, makes #isMyRole largely irrelevant
  • RoleManager#premiumSubscriberRole ✔ easy access for a guilds premium subscription role
  • RoleManager#myRole ❌
  • RoleManager#botRoleFor(UserResolvable) ✔ can act as more general replacement for #myRole
  • Integration#roles ✔ Integration#role just covers one of the possibly many integration roles
Past caveats and roadblocks

Status

  • 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

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.

src/managers/RoleManager.js Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
src/structures/Role.js Outdated Show resolved Hide resolved
src/structures/Role.js Outdated Show resolved Hide resolved
src/managers/RoleManager.js Outdated Show resolved Hide resolved
src/managers/GuildMemberRoleManager.js Outdated Show resolved Hide resolved
src/structures/Role.js Outdated Show resolved Hide resolved
@Androz2091
Copy link
Sponsor Contributor

Androz2091 commented Jul 11, 2020

There is no way to get the role of a bot from a GuildMember instance, except for the client?
Something like:

const infoEmbed = new MessageEmbed();
if(member.user.bot){
    infoEmbed.addField('Role created for this bot', member.roles.itsRole.toString());
}
// this could simplify userinfo commands

For the moment, this can be found using:

const infoEmbed = new MessageEmbed();
if(member.user.bot){
    infoEmbed.addField('Role created for this bot', member.roles.cache.find((r) => r.tags.botID === member.id).toString());
}

Maybe I'm wrong but as a property exists to find the role of the client, it seems logical to me to have a getter to easily get the role of the other bots

@papaia
Copy link
Contributor

papaia commented Jul 11, 2020

@Androz2091

const botRole = roles.find(r => r.tags?.botID === botID);

Another getter, especially if named itsRole, is redundant.
It would essentially mean that every GuildMember will have this getter, bot or not bot, and itsRole is very confusing.

But
It could be nice if integrations had a role getter for them.

@almostSouji
Copy link
Member Author

almostSouji commented Jul 11, 2020

  • I can see <RoleManager>.botRole(<UserResolvable>) to make that slightly easier to access (note: this is Guild#roles, not GuildMember#roles)
  • I personally prefer Integration#role over <RoleManager>.integrationRole(<Integration|Snowflake>) , though the latter might also have its place for consistencies sake. already exists
  • GuildMember#roles#botRole is a much nicer API for the user. going with that one

typings/index.d.ts Outdated Show resolved Hide resolved
src/structures/Role.js Outdated Show resolved Hide resolved
src/structures/Role.js Outdated Show resolved Hide resolved
src/structures/Role.js Outdated Show resolved Hide resolved
@almostSouji
Copy link
Member Author

almostSouji commented Jul 11, 2020

This API has become very convoluted, i don't think this is necessary or exactly beneficial and will trim it back with the following considerations:

  • Role#tags ✔ fundamental
  • Role#isMyRole ❌ was only used internally, feels clunky
  • Role#isPremiumSubscriberRole ❌ was only used internally, feels clunky
  • GuildMemberRoleManager#premiumSubscriberRole ✔ permission check
  • GuildMemberRoleManager#botRole ✔ easy access for a certain bots role, makes #isMyRole largely irrelevant
  • RoleManager#premiumSubscriberRole ✔ easy access for a guilds premium subscription role
  • RoleManager#myRole ❌
  • RoleManager#botRoleFor(UserResolvable) ✔ can act as more general replacement for #myRole

typings/index.d.ts Outdated Show resolved Hide resolved
@almostSouji
Copy link
Member Author

@Androz2091 thanks for the suggestion, this is now in as member.roles.botRole. (the members bot role, getter)
If you want the role for other bots it's also available through guild.roles.botRoleFor(botUser)

I think this is now both usable as well as verbosely named.

I will also add another consideration note to the bunch above regarding #botRoleFor not being called on the #botRole getter

@almostSouji

This comment has been minimized.

@discordjs discordjs deleted a comment from almostSouji Jul 14, 2020
@almostSouji

This comment has been minimized.

@Skye-31
Copy link
Contributor

Skye-31 commented Nov 25, 2020

discord/discord-api-docs#1537 (comment) reply rollout is complete

@almostSouji
Copy link
Member Author

almostSouji commented Nov 25, 2020

If there is any feedback from contribs/maintainers regarding the proposed API or implementation do let me know here or on discord - as you prefer and expect interaction to be necessary.

could consider removing

  • GuildMemberRoleManager#premiumSubscriberRole
  • GuildMemberRoleManager#botRole

For less getter clutter and let these be done by the user instead through <GuildMember>.guild.roles.botRoleFor(<GuildMember>)/premiumSubscriberRole and then look for that through <GuildMember>.roles.has(<role>.id)

@almostSouji almostSouji marked this pull request as ready for review November 25, 2020 21:08
@almostSouji

This comment has been minimized.

src/structures/Role.js Outdated Show resolved Hide resolved
@kyranet kyranet requested a review from iCrawl December 14, 2020 02:26
@iCrawl iCrawl merged commit d6234b7 into discordjs:master Dec 14, 2020
@almostSouji almostSouji deleted the feat-role-tags branch December 14, 2020 16:52
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

10 participants