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 tags to role model #230

Merged
merged 5 commits into from
Jun 28, 2023
Merged

Add tags to role model #230

merged 5 commits into from
Jun 28, 2023

Conversation

gracz21
Copy link
Contributor

@gracz21 gracz21 commented Jun 27, 2023

Summary

I added a wrapper class for the Discord role's tags and added the tags field to the role model as it wasn't possible to access the role tags data (e.g. role's bot ID) while using the server role created/updated/deleted gateway event payload.


Added

  • added Discordrb::Role::Tags class

Changed

  • added Discordrb::Role#tags attribute

Copy link
Contributor

@Dakurei Dakurei left a comment

Choose a reason for hiding this comment

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

Error in returned type according to documentation (String instead of Integer)

lib/discordrb/data/role.rb Outdated Show resolved Hide resolved
lib/discordrb/data/role.rb Outdated Show resolved Hide resolved
lib/discordrb/data/role.rb Outdated Show resolved Hide resolved
@gracz21
Copy link
Contributor Author

gracz21 commented Jun 27, 2023

Thanks for review @Dakurei. All suggestions were applied

@gracz21 gracz21 requested a review from Dakurei June 27, 2023 17:54
Copy link
Contributor

@Dakurei Dakurei left a comment

Choose a reason for hiding this comment

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

Personally, it looks fine to me, I don't have access to roles with "integration_id", "subscription_listing_id", "available_for_purchase" and "guild_connections" tags to check absolutely everything in terms of the content returned, but there's no reason for any problems 👍

I'll pass the baton to anyone who wants to check this out in more detail !

Copy link
Member

@swarley swarley left a comment

Choose a reason for hiding this comment

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

Looks good, thank you both.

@swarley swarley merged commit b029e52 into shardlab:main Jun 28, 2023
@gracz21
Copy link
Contributor Author

gracz21 commented Jun 28, 2023

Cool, thanks

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.

3 participants