-
-
Notifications
You must be signed in to change notification settings - Fork 129
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(model, cache): Support new username system #2231
feat(model, cache): Support new username system #2231
Conversation
…eat/new-usernames
This seems to contain some changes unrelated to the user name system, can we put that in a seperate PR if there isn't one already? |
Whoops looks my changes in #2222 got in this branch, I'll fix this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
29 of 32 files are just model testing code updates lol. I guess that's a good thing.
i think we should hold this until the docs are deployed again |
should we include the change of making |
It's not possible to have a #0000 discriminator |
The docs have already been deployed for this https://discord.com/developers/docs/resources/user#user-object |
yes it is, for example webhooks have it |
ah my bad, coulda sworn it wasnt there but i was probably just blind |
Webhooks aren’t considered users they’re just apps. Discord showing #0000 for their discriminator in the client is a fallback since it doesn’t have a discriminator from the API. |
its pretty common for the lib user to get a |
Yes, however those webhooks have non #0000 discriminators. |
running a quick |
I just tested as well, I was mistaken. You are correct it does return 0000 in those situations. So I suppose we should convert the discriminator to a string. |
May be more efficient to do something like ImageHash (https://github.com/twilight-rs/twilight/blob/main/twilight-model/src/util/image_hash.rs) Eg struct Discriminator {
// Same as current u16 deserialization implementation
inner: u16,
// If webhook (0000) then DiscriminatorDisplay can pad accordingly otherwise just return 0 for pomelo users
webhook: bool
} Should be a few bytes compared to like 20 something with a string unless I'm calculating it all wrong (very possible as it's late and I'm dumb 🤣) |
As long as webhook's
While this is a possible representation, I don't think we should be expand the API for this legacy feature. Very very few users will still have discriminators by the time twilight 0.16 releases. Heck, even now few users still have a discriminator. |
I'm strongly against using strings for discriminators for caching reasons, it would blow up the size of user objects a fair bit. I'd either keep it as is or use an approach like @ImDarkDiamond suggested |
i agree with @ImDarkDiamond's approach, since people can check |
Possible changes to the discriminator representation in model should go into a separate PR. |
This adds support for the
global_name
field which is a part of the larger username migration.One other notable change with this PR is that the max size of an
Event
is no longer192
but now203
to accommodate for the new field.Ref: