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.

Sign up for GitHub

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

Jump to bottom

feat: ✨ New role tags #2606

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

feat: ✨ New role tags #2606

wants to merge 10 commits into from

Conversation

Paillat-dev
Copy link
Contributor

@Paillat-dev Paillat-dev commented Oct 11, 2024 •
edited
Loading

Summary

fixes: #2474
cc @Lumabots

Information

  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed).
  • This PR is not a code change (e.g. documentation, README, typehinting,
    examples, ...).

Checklist

  • I have searched the open pull requests for duplicates.
  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why.
  • I have updated the changelog to include these changes.

@Paillat-dev Paillat-dev requested a review from a team as a code owner October 11, 2024 10:03
@pullapprove4 pullapprove4 bot requested a review from FrostByte266 October 11, 2024 10:03
@Paillat-dev Paillat-dev changed the title 🩹 Fix Role.is_integration() Fix 🩹 Role.is_integration() Oct 11, 2024
@Paillat-dev Paillat-dev changed the title Fix 🩹 Role.is_integration() fix: 🩹 Role.is_integration() Oct 11, 2024
@Lulalaby
Copy link
Member

Changelog

@Paillat-dev Paillat-dev force-pushed the fix-is-integration branch 2 times, most recently from 9270a61 to fb32e22 Compare October 11, 2024 16:06
@Paillat-dev
Copy link
Contributor Author

There. Maybe discord.RoleTags.is_integration() should be removed too ? That would be breaking tho.

@plun1331
Copy link
Member

plun1331 commented Oct 11, 2024 •
edited
Loading

is_integration is for roles managed by integrations, such as patreon/twitch
is_bot_managed is for roles assigned to bots when added to a server by discord
is_premium_subscriber is for the server booster role managed by discord
managed is True if any of the above is True

In my testing, with this logic, is_integration works just fine and was never broken.
This should really be a documentation update, as the docs are misleading.

@Lulalaby
Copy link
Member

Do we cover the new tags also? Like for role subscription, server shop, etc

@plun1331
Copy link
Member

Do we cover the new tags also? Like for role subscription, server shop, etc

We do not, missing subscription_listing_id, available_for_purchase, guild_connections

@Lulalaby
Copy link
Member

https://gist.github.com/Lulalaby/a9ac5fcce5c182efd55167e6141c1542
As little help

@Paillat-dev
Copy link
Contributor Author

Yeah that was a documentation issue. I will change that instead.

@Paillat-dev Paillat-dev marked this pull request as draft October 11, 2024 20:17
@plun1331 plun1331 added documentation Improvements or additions to documentation priority: medium Medium Priority status: in progress Work in Progess feature Implements a feature API Reflection Discords API wasn't correctly reflected in the lib and removed on hold labels Oct 11, 2024
@plun1331 plun1331 added this to the v2.7 milestone Oct 11, 2024
@Paillat-dev
Copy link
Contributor Author

@plun1331 I might add the other tags too now that i'm here.

@plun1331
Copy link
Member

Go for it, shouldn't be too difficult

Lulalaby
Lulalaby requested changes Oct 21, 2024
View reviewed changes
Copy link
Member

@Lulalaby Lulalaby left a comment

Choose a reason for hiding this comment

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

Do not merge without my approval

@Paillat-dev Paillat-dev requested a review from Lulalaby October 26, 2024 21:35
@plun1331
Copy link
Member

do you have to force push everything

@Paillat-dev
Copy link
Contributor Author

I like keeping a clean history

@plun1331
Copy link
Member

plun1331 commented Nov 2, 2024

I like keeping a clean history

makes it harder to see what you changed, and it all gets squashed anyways

JustaSqu1d
JustaSqu1d suggested changes Dec 12, 2024
View reviewed changes
Copy link
Member

@JustaSqu1d JustaSqu1d left a comment

Choose a reason for hiding this comment

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

grammar stuff

discord/role.py Outdated Show resolved Hide resolved
discord/role.py Outdated Show resolved Hide resolved
discord/role.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
JustaSqu1d
JustaSqu1d suggested changes Dec 17, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
discord/role.py Outdated Show resolved Hide resolved
discord/role.py Outdated Show resolved Hide resolved
discord/role.py Outdated Show resolved Hide resolved
discord/role.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dorukyum Dorukyum Dorukyum left review comments

@plun1331 plun1331 plun1331 left review comments

@Lulalaby Lulalaby Awaiting requested review from Lulalaby

@JustaSqu1d JustaSqu1d Awaiting requested review from JustaSqu1d

Requested changes must be addressed to merge this pull request.

Assignees

@Lulalaby Lulalaby

Labels
API Reflection Discords API wasn't correctly reflected in the lib documentation Improvements or additions to documentation feature Implements a feature PA: All Contributors pending PA: Maintainers pending priority: medium Medium Priority status: in progress Work in Progess
Projects
None yet
Milestone
v2.7
Development

Successfully merging this pull request may close these issues.

is_integration return False even if its an integration
5 participants
@Paillat-dev @Lulalaby @plun1331 @Dorukyum @JustaSqu1d