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 :global_name for Nostrum.Struct.User #491

Merged
merged 5 commits into from
Jun 8, 2023

Conversation

Ovyerus
Copy link
Contributor

@Ovyerus Ovyerus commented May 21, 2023

Ref: discord/discord-api-docs#6130

Adds :global_name to the user struct to support the new username & display name/"pomelo" system for Discord.

Also updates User.full_name/1 to show only the global name if present (otherwise falling back to the old Username#1234), and User.avatar_url/1 to use the new method of calculating the default avatar URL for a user if their discriminator is missing (ie. "0") (reference: https://github.com/discord/discord-api-docs/blob/b7a4d80861aea9e116d123a253acd460386a89b5/docs/Change_Log.md?plain=1#L45-L47)

iex(2)> alias Nostrum.Struct.User
Nostrum.Struct.User
iex(3)> User.full_name(%User{global_name: "Rie", username: "rie_t", discriminator: "0"})
"Rie"
iex(4)> User.full_name(%User{global_name: nil, username: "Ovyerus", discriminator: "2211"})
"Ovyerus#2211"

Copy link
Collaborator

@jchristgit jchristgit left a comment

Choose a reason for hiding this comment

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

Thank you!

I will merge this if Discord decides to go forward with the change, given the upstream docs change is not fully merged yet, a part of me is still hoping they don't decide on implementing it after all. Let's see what happens.

@chimpdev
Copy link

Thank you!

I will merge this if Discord decides to go forward with the change, given the upstream docs change is not fully merged yet, a part of me is still hoping they don't decide on implementing it after all. Let's see what happens.

They will not stop what they are doing.

@bdanklin
Copy link
Contributor

Thank you!
I will merge this if Discord decides to go forward with the change, given the upstream docs change is not fully merged yet, a part of me is still hoping they don't decide on implementing it after all. Let's see what happens.

They will not stop what they are doing.

just like they didn't stop their plans for using discord as a game distribution platform..... oh wait

@Ovyerus
Copy link
Contributor Author

Ovyerus commented Jun 8, 2023

Looks like the API changes got moved to discord/discord-api-docs#6218 which has since been merged.

(Had to update to avatar calculation since users in the new system get a new pink avatar, dunno what happened with the commit history in here, thanks GitHub)

@jchristgit
Copy link
Collaborator

Looks like the API changes got moved to discord/discord-api-docs#6218 which has since been merged.

(Had to update to avatar calculation since users in the new system get a new pink avatar, dunno what happened with the commit history in here, thanks GitHub)

Fair enough. Then I think we can merge it. But I need a clean history. Can you try a rebase or should I try on a separate branch?

@Ovyerus
Copy link
Contributor Author

Ovyerus commented Jun 8, 2023

I'll try a rebase

@Ovyerus Ovyerus force-pushed the feat/global-names branch from 6060a39 to dcf77dd Compare June 8, 2023 06:37
@Ovyerus
Copy link
Contributor Author

Ovyerus commented Jun 8, 2023

Ok should be good now, I did do a rebase last time but I'm not sure what goofed up.

Copy link
Collaborator

@jchristgit jchristgit left a comment

Choose a reason for hiding this comment

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

Thanks!

@jchristgit
Copy link
Collaborator

       "https://cdn.discordapp.com/embed/avatars/5.png"
     code:  Nostrum.Struct.User.avatar_url(user) === "https://cdn.discordapp.com/embed/avatars/5.png"
     left:  "https://cdn.discordapp.com/embed/avatars/4.png"
     right: "https://cdn.discordapp.com/embed/avatars/5.png"
     stacktrace:

Is this test failure due to the pink avatar changes?

@Ovyerus
Copy link
Contributor Author

Ovyerus commented Jun 8, 2023

I think so yea. Also looks like the last 2 commits from the rebase are screwed up for some reason. Will fix!

Looks like it was just those weird commits causing it.

@Ovyerus Ovyerus force-pushed the feat/global-names branch from dcf77dd to 8b84fda Compare June 8, 2023 07:15
Copy link
Collaborator

@jchristgit jchristgit left a comment

Choose a reason for hiding this comment

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

Thanks!

@jchristgit jchristgit merged commit 75527dd into Kraigie:master Jun 8, 2023
@jchristgit
Copy link
Collaborator

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.

4 participants