twilight-rs / twilight

Powerful, flexible, and scalable ecosystem of Rust libraries for the Discord API.
https://discord.gg/twilight-rs
ISC License
673 stars 132 forks source link

feat(model, cache): Support new username system #2231

Closed suneettipirneni closed 1 year ago

suneettipirneni commented 1 year ago

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 longer 192 but now 203 to accommodate for the new field.

Ref:

Gelbpunkt commented 1 year ago

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?

suneettipirneni commented 1 year ago

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.

laralove143 commented 1 year ago

i think we should hold this until the docs are deployed again

laralove143 commented 1 year ago

should we include the change of making discriminator a String here? so that #0 and #0000 are differentiable

vilgotf commented 1 year ago

should we include the change of making discriminator a String here? so that #0 and #0000 are differentiable

It's not possible to have a #0000 discriminator

suneettipirneni commented 1 year ago

i think we should hold this until the docs are deployed again

The docs have already been deployed for this https://discord.com/developers/docs/resources/user#user-object

laralove143 commented 1 year ago

should we include the change of making discriminator a String here? so that #0 and #0000 are differentiable

It's not possible to have a #0000 discriminator

yes it is, for example webhooks have it

laralove143 commented 1 year ago

i think we should hold this until the docs are deployed again

The docs have already been deployed for this https://discord.com/developers/docs/resources/user#user-object

ah my bad, coulda sworn it wasnt there but i was probably just blind

suneettipirneni commented 1 year ago

should we include the change of making discriminator a String here? so that #0 and #0000 are differentiable

It's not possible to have a #0000 discriminator

yes it is, for example webhooks have it

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.

laralove143 commented 1 year ago

its pretty common for the lib user to get a User for a webhook, for example with message.author

suneettipirneni commented 1 year ago

its pretty common for the lib user to get a User for a webhook, for example with message.author

Yes, however those webhooks have non #0000 discriminators.

laralove143 commented 1 year ago

running a quick GET /channels/cid/messages/mid request on a webhook message responds with a json whose author.discriminator is "0000"

suneettipirneni commented 1 year ago

running a quick GET /channels/cid/messages/mid request on a webhook message responds with a json whose author.discriminator is "0000"

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.

ImDarkDiamond commented 1 year ago

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 🤣)

vilgotf commented 1 year ago

As long as webhook's User object differs from normal users' it should be fine to represent both as 0. Won't the bot field be set to true for webhooks?

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
}

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.

Gelbpunkt commented 1 year ago

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

laralove143 commented 1 year ago

i agree with @ImDarkDiamond's approach, since people can check global_username, differentiating between 0 and 0000 isnt very necessary but i think we should follow discord since thats a goal of the library

Gelbpunkt commented 1 year ago

Possible changes to the discriminator representation in model should go into a separate PR.