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: support new username system #1025

Merged
merged 27 commits into from May 25, 2023
Merged

feat: support new username system #1025

merged 27 commits into from May 25, 2023

Conversation

shiftinv
Copy link
Member

@shiftinv shiftinv commented May 6, 2023

Summary

Initial implementation of the recently announced username system changes. See discord/discord-api-docs#6130.

The changelog entry should give a pretty solid overview over the specific changes.
Discriminator handling in converters and Guild.get_member_named is kept for now, it's planned to be removed once the username migration is complete.

There are a couple open issues and things that need to be discussed further:

TBD

  • Documentation phrasing: "global name" vs "global nickname" vs "global display name" vs ...
  • Should the discriminator= field be removed from __repr__s?
    ~ decided to not remove it, for now
  • Should User.__str__ return name or @name?
    c7f05b0
  • Do we want to handle the case of user#0 in converters + get_member_named for migrated users?
    df79046 + c8d8d8d
  • The discriminator field may disappear in future API versions; we may want to add a "0" fallback for this already for future proofing, even if it should (ahem) be bound to specific API versions
    ~ also not doing this for now, see feat: support new username system #1025 (comment)
  • The future of widget members is uncertain, for now this implementation assumes that the received username already accounts for the three different names a user can have

Checklist

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running pdm lint
    • I have type-checked the code by running pdm pyright
  • 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, ...)

@shiftinv shiftinv added t: enhancement New feature t: api support Support of Discord API features p: high High priority s: in progress Issue/PR is being worked on s: waiting for api/docs Issue/PR is waiting for API support/documentation labels May 6, 2023
@shiftinv shiftinv added this to the disnake v2.9 milestone May 6, 2023
Copy link
Member

@Victorsitou Victorsitou left a comment

Choose a reason for hiding this comment

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

quick review, documentation looks fine overall c:

disnake/user.py Outdated Show resolved Hide resolved
@shiftinv shiftinv added the s: needs review Issue/PR is awaiting reviews label May 16, 2023
Copy link
Member

@onerandomusername onerandomusername left a comment

Choose a reason for hiding this comment

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

Thoughts on a possible field or helper for determining if a user is on the new system without checking the deprecated discriminator field? Maybe we don't deprecate the discriminator field right now?

changelog/1025.feature.rst Outdated Show resolved Hide resolved
disnake/abc.py Outdated Show resolved Hide resolved
disnake/abc.py Outdated Show resolved Hide resolved
disnake/member.py Show resolved Hide resolved
disnake/team.py Show resolved Hide resolved
disnake/types/user.py Show resolved Hide resolved
disnake/user.py Show resolved Hide resolved
disnake/user.py Show resolved Hide resolved
disnake/user.py Show resolved Hide resolved
disnake/user.py Outdated Show resolved Hide resolved
@shiftinv shiftinv marked this pull request as ready for review May 18, 2023 22:20
disnake/user.py Outdated Show resolved Hide resolved
@shiftinv shiftinv removed the s: in progress Issue/PR is being worked on label May 20, 2023
@shiftinv shiftinv removed the s: waiting for api/docs Issue/PR is waiting for API support/documentation label May 21, 2023
@shiftinv
Copy link
Member Author

shiftinv commented May 21, 2023

This should be ready to go now, I've done a bunch more testing. Since the introduction of global_name is a fairly significant ecosystem change, I'm not going to wait for the api docs to be merged here. Reviews/Testing would really be appreciated nonetheless :>
I don't think we're going to cut a new release immediately, so if there really is anything that needs to be changed later there's still time for that.

Copy link
Member

@Victorsitou Victorsitou 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 to me

disnake/abc.py Show resolved Hide resolved
@shiftinv shiftinv enabled auto-merge (squash) May 25, 2023 21:56
@shiftinv shiftinv merged commit 7a6880e into master May 25, 2023
24 checks passed
@shiftinv shiftinv deleted the feature/pomelo branch May 25, 2023 21:56
The user's global display name, if set.
This takes precedence over :attr:`.name` when shown.

For bots, this is the application name.
Copy link
Member

Choose a reason for hiding this comment

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

just triple checked and this is currently not true 🦀

Copy link
Member Author

@shiftinv shiftinv May 27, 2023

Choose a reason for hiding this comment

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

yeah :/ It was announced back then, which is why I originally included it in this PR. It'll be correct eventually :p
image
source

edit: welp.

@onerandomusername onerandomusername removed the s: needs review Issue/PR is awaiting reviews label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p: high High priority t: api support Support of Discord API features t: enhancement New feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants