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 #1387

Merged
merged 19 commits into from Jul 4, 2023

Conversation

Aldiwildan77
Copy link
Contributor

@Aldiwildan77 Aldiwildan77 commented Jun 8, 2023

Problem

closes #1386

Unit Test Check

image

image

user.go Show resolved Hide resolved
Copy link

@RiskyFeryansyahP RiskyFeryansyahP left a comment

Choose a reason for hiding this comment

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

nit

user.go Show resolved Hide resolved
RiskyFeryansyahP

This comment was marked as resolved.

user.go Outdated Show resolved Hide resolved
user.go Outdated Show resolved Hide resolved
Aldiwildan77 and others added 2 commits June 10, 2023 22:30
Co-authored-by: Fedor Lapshin <fe.lap.prog@gmail.com>
Co-authored-by: Fedor Lapshin <fe.lap.prog@gmail.com>
Copy link
Collaborator

@FedorLap2006 FedorLap2006 left a comment

Choose a reason for hiding this comment

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

While I don't think we had done this before, I think it's a good idea to put all table tests under a subtest (with the name being something along the lines of "String()" or "Conversion to a string").

user_test.go Outdated Show resolved Hide resolved
user_test.go Outdated Show resolved Hide resolved
user_test.go Outdated Show resolved Hide resolved
user_test.go Outdated Show resolved Hide resolved
@FedorLap2006
Copy link
Collaborator

Also, please note the checks.

@FedorLap2006 FedorLap2006 added feature Feature implementation high priority Issue or PR with high priority of merge structs API structs and constants related changes labels Jun 10, 2023
@FedorLap2006 FedorLap2006 added this to the v0.28.0 milestone Jun 10, 2023
Aldiwildan77 and others added 3 commits June 11, 2023 07:00
Co-authored-by: Fedor Lapshin <fe.lap.prog@gmail.com>
Co-authored-by: Fedor Lapshin <fe.lap.prog@gmail.com>
Co-authored-by: Fedor Lapshin <fe.lap.prog@gmail.com>
@Aldiwildan77
Copy link
Contributor Author

While I don't think we had done this before, I think it's a good idea to put all table tests under a subtest (with the name being something along the lines of "String()" or "Conversion to a string").

yap, do we need a standardization related to the unit test? I think all features will need this since the package become bigger

change `tt` to `tc`. the purpose is to make the variable better communicate.

Co-authored-by: Fedor Lapshin <fe.lap.prog@gmail.com>
@Aldiwildan77
Copy link
Contributor Author

any update for this @FedorLap2006 ?

@cheesycod
Copy link
Contributor

Same, this is kinda needed because of global display names

@Aldiwildan77
Copy link
Contributor Author

I've updated this code, please review it again. Thanks, @FedorLap2006

user.go Outdated Show resolved Hide resolved
user.go Outdated Show resolved Hide resolved
@FedorLap2006
Copy link
Collaborator

FedorLap2006 commented Jun 27, 2023

Additionally, not quite sure about the separation of EndpointDefaultAvatar into two funcs. There's an alternative solution, which would match API more here - supplying an index into a single EndpointDefaultAvatar func and computing it in AvatarURL.

However, it's quite late, so I'll decide on that tomorrow.

Co-authored-by: Fedor Lapshin <fe.lap.prog@gmail.com>
@Aldiwildan77
Copy link
Contributor Author

Additionally, not quite sure about the separation of EndpointDefaultAvatar into two funcs. There's an alternative solution, which would match API more here - supplying an index into a single EndpointDefaultAvatar func and computing it in AvatarURL.

However, it's quite late, so I'll decide on that tomorrow.

okay, so keep the EndpointDefaultAvatar and filter on those function right?

endpoints.go Outdated Show resolved Hide resolved
user.go Outdated Show resolved Hide resolved
@FedorLap2006
Copy link
Collaborator

FedorLap2006 commented Jun 29, 2023

Additionally, not quite sure about the separation of EndpointDefaultAvatar into two funcs. There's an alternative solution, which would match API more here - supplying an index into a single EndpointDefaultAvatar func and computing it in AvatarURL.
However, it's quite late, so I'll decide on that tomorrow.

okay, so keep the EndpointDefaultAvatar and filter on those function right?

Basically, but with a couple more changes. See my comment above.

@Aldiwildan77
Copy link
Contributor Author

I've updated this code, please review it again. Thanks, @FedorLap2006

user.go Outdated Show resolved Hide resolved
@FedorLap2006
Copy link
Collaborator

FedorLap2006 commented Jul 3, 2023

I might make the change myself tho, since I understand that everyone wants to have this change 😅

@FedorLap2006 FedorLap2006 changed the title feat: support new username only feat: support new username system Jul 3, 2023
@Aldiwildan77
Copy link
Contributor Author

I might make the change myself tho, since I understand that everyone wants to have this change 😅

😅Hahah thanks for the changes. Glad to know for the new update

Use regular int instead of uint64 for return value of
User.DefaultAvatarIndex and idx parameter of EndpointDefaultUserAvatar.
@FedorLap2006
Copy link
Collaborator

FedorLap2006 commented Jul 4, 2023

Want to give a bit of explanation behind my last commit:

After reading into the documentation and asking around, I've found that user should have only "0" discriminator, therefore I decided to remove the handling (and tests) of "" case, alongside isMigrated func, since it's not going to be needed for a simple == check

Additionally, with a bit of consideration, I've decided to move the comment, since it's probably going to be better if we explain the "0" case, instead of the normal one, which everyone is already used to.

@FedorLap2006
Copy link
Collaborator

And this one - I decided to switch the article to more general version, but decided to undo it, as the "0" part is explained only in developer version

@FedorLap2006 FedorLap2006 merged commit e39e715 into bwmarrin:master Jul 4, 2023
8 checks passed
@FedorLap2006
Copy link
Collaborator

Thanks for your contribution!

@Aldiwildan77
Copy link
Contributor Author

Want to give a bit of explanation behind my last commit:

After reading into the documentation and asking around, I've found that user should have only "0" discriminator, therefore I decided to remove the handling (and tests) of "" case, alongside isMigrated func, since it's not going to be needed for a simple == check

Additionally, with a bit of consideration, I've decided to move the comment, since it's probably going to be better if we explain the "0" case, instead of the normal one, which everyone is already used to.

I see, so just simply compare for the "0" value of the discriminator.

@Aldiwildan77
Copy link
Contributor Author

Thanks for your contribution!

Ur welcome, glad to know if there are many cases that can be improved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature implementation high priority Issue or PR with high priority of merge structs API structs and constants related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User string got #0 discriminator caused by discord update
5 participants