Skip to content

refactor: Rewrite the role tags mess #2708

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

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

Paillat-dev
Copy link
Contributor

@Paillat-dev Paillat-dev commented Feb 6, 2025

Summary

Rewrite the role tags mess.
image

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 force-pushed the feat/role-tags-rewrite branch from 20d75bb to 696dcbf Compare February 6, 2025 09:02
@Paillat-dev Paillat-dev changed the title refactor: Rewrite the role tags mess refactor!: Rewrite the role tags mess Feb 6, 2025
Copy link
Member

@plun1331 plun1331 left a comment

Choose a reason for hiding this comment

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

This looks really easy to make backwards compatible, can we just do that and deprecate all the compatibility methods?

@Paillat-dev
Copy link
Contributor Author

Yeah maybe i'll try, but it sucks when I reused method names to become properties

@Paillat-dev
Copy link
Contributor Author

What are your thoughts on adding something like an enum, e.g. Role.type to describe the role type ?

@Lulalaby
Copy link
Member

As we previously talked on dcs, the role type enum is the best way. Like plun said, deprecated all current methods and just create the type on role. But the docs should clarify that it's calculated since it's not returned by discord

@Dorukyum Dorukyum self-requested a review February 13, 2025 08:54
@Paillat-dev Paillat-dev force-pushed the feat/role-tags-rewrite branch 2 times, most recently from 0c18960 to 7e7d40e Compare February 15, 2025 13:28
@Paillat-dev Paillat-dev force-pushed the feat/role-tags-rewrite branch from 7e7d40e to 5810fa3 Compare February 15, 2025 13:29
@Paillat-dev Paillat-dev force-pushed the feat/role-tags-rewrite branch from 27a15ee to 54f1baa Compare February 15, 2025 13:54
@Paillat-dev Paillat-dev changed the title refactor!: Rewrite the role tags mess refactor: Rewrite the role tags mess Feb 15, 2025
@Paillat-dev
Copy link
Contributor Author

Paillat-dev commented Feb 15, 2025

Here is a handy snippet for testing this:

import logging
import os
from datetime import datetime, UTC
from dotenv import load_dotenv

import discord

load_dotenv()

logging.basicConfig(level=logging.DEBUG)

TOKEN = os.getenv("TOKEN_3")

bot = discord.Bot()


@bot.slash_command()
async def test_roles(ctx: discord.ApplicationContext, role: discord.Role):
    await ctx.defer()
    if not ctx.guild:
        return await ctx.respond("This command must be used in a guild.")
    fetched_role = await ctx.guild.fetch_role(role.id)
    await ctx.respond(f"""```
Role:
    {role!r}
Role tags:
    {role.tags!r}
Role tags data:
    {role.tags._data if role.tags else None}

Fetched role:
    {fetched_role!r}
Fetched role tags:
    {fetched_role.tags!r}
Fetched role tags data:
    {fetched_role.tags._data if fetched_role.tags else None}       
```""")

bot.run(TOKEN)

@Paillat-dev
Copy link
Contributor Author

Do not merge yet

@Paillat-dev
Copy link
Contributor Author

Missing testing is only twitch/ytb roles, everything else is tested.

@Paillat-dev Paillat-dev requested a review from plun1331 February 21, 2025 16:41
def is_guild_connections_role(self) -> bool:
"""Whether the role is a guild connections role.

Copy link
Contributor Author

@Paillat-dev Paillat-dev Mar 2, 2025

Choose a reason for hiding this comment

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

Wait this could be removed if this pr is merged before 2.7 since it was added after 2.6.1. Same for is_available_for_purchase

@Paillat-dev
Copy link
Contributor Author

Any news on this topic ?

@Paillat-dev
Copy link
Contributor Author

@Lulalaby Any news on this btw ?

@Lulalaby
Copy link
Member

not yet :(

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