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(model, cache): Support new username system #2231

Merged
merged 7 commits into from
Jul 9, 2023

Conversation

suneettipirneni
Copy link
Member

@suneettipirneni suneettipirneni commented Jul 1, 2023

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:

@github-actions github-actions bot added c-cache Affects the cache crate c-model Affects the model crate c-standby Affects the standby crate t-feature Addition of a new feature labels Jul 1, 2023
@suneettipirneni suneettipirneni self-assigned this Jul 1, 2023
@suneettipirneni suneettipirneni requested a review from a team July 1, 2023 19:53
@Gelbpunkt
Copy link
Member

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
Copy link
Member Author

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.

Copy link
Member

@vilgotf vilgotf left a 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.

twilight-model/src/user/mod.rs Outdated Show resolved Hide resolved
twilight-model/src/user/mod.rs Outdated Show resolved Hide resolved
twilight-model/src/user/mod.rs Outdated Show resolved Hide resolved
@vilgotf vilgotf enabled auto-merge (squash) July 1, 2023 20:32
@vilgotf vilgotf disabled auto-merge July 1, 2023 20:32
@laralove143
Copy link
Member

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

@laralove143
Copy link
Member

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

@vilgotf
Copy link
Member

vilgotf commented Jul 1, 2023

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

suneettipirneni commented Jul 2, 2023

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
Copy link
Member

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

@suneettipirneni
Copy link
Member Author

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
Copy link
Member

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

@suneettipirneni
Copy link
Member Author

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
Copy link

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
Copy link
Member

vilgotf commented Jul 2, 2023

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

@Gelbpunkt Gelbpunkt merged commit 4a1937e into twilight-rs:main Jul 9, 2023
10 checks passed
@suneettipirneni suneettipirneni deleted the feat/new-usernames branch July 9, 2023 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-cache Affects the cache crate c-model Affects the model crate c-standby Affects the standby crate t-feature Addition of a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants