Skip to content

refactor: Rewrite the role tags mess#2708

Merged
Lulalaby merged 46 commits into
Pycord-Development:masterfrom
Paillat-dev:feat/role-tags-rewrite
Mar 8, 2026
Merged

refactor: Rewrite the role tags mess#2708
Lulalaby merged 46 commits into
Pycord-Development:masterfrom
Paillat-dev:feat/role-tags-rewrite

Conversation

@Paillat-dev

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

Copy link
Copy Markdown
Member

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

@plun1331 plun1331 left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Member Author

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

@Paillat-dev

Copy link
Copy Markdown
Member Author

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

Comment thread discord/role.py
@Lulalaby

Copy link
Copy Markdown
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

Paillat-dev commented Feb 15, 2025

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Member Author

Do not merge yet

@Paillat-dev
Paillat-dev force-pushed the feat/role-tags-rewrite branch from d030f17 to c27c52d Compare February 18, 2026 13:31
@Paillat-dev
Paillat-dev force-pushed the feat/role-tags-rewrite branch from 3e162b1 to cab1fe7 Compare February 18, 2026 13:38
@Paillat-dev

Copy link
Copy Markdown
Member Author

Was also able to test with twitch / youtube roles in a community I manage and it seems to work fine too.
image

Comment thread discord/role.py
Comment thread discord/role.py Outdated
Comment thread discord/role.py Outdated
Comment thread discord/role.py
Comment thread discord/role.py Outdated
@Paillat-dev
Paillat-dev requested a review from Soheab February 20, 2026 15:06
Comment thread discord/types/role.py Outdated

@Lulalaby Lulalaby left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hate this

@Lulalaby
Lulalaby merged commit a5e556f into Pycord-Development:master Mar 8, 2026
27 checks passed
@plun1331

plun1331 commented Mar 8, 2026

Copy link
Copy Markdown
Member

the fuck did i do

@Icebluewolf

Copy link
Copy Markdown
Member

So, it's too late to be non-breaking now, but all other enums in pycord are lowercase while the enum created here is uppercase.

@Paillat-dev

Copy link
Copy Markdown
Member Author

Uppercase is more idiomatic because they are constants, so it's not that bad. I personally think we should make all of them uppercase, and deprecate the lowercase ones but it's a discussion for another time & place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

discord limitation Limitation imposed by discord priority: low Low Priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants