refactor!(model, cache, gateway): Make Presence::guild_id an Option - #2125
Open
suneettipirneni wants to merge 3 commits into
Open
refactor!(model, cache, gateway): Make Presence::guild_id an Option#2125suneettipirneni wants to merge 3 commits into
Presence::guild_id an Option#2125suneettipirneni wants to merge 3 commits into
Conversation
github-actions
Bot
added
c-cache
suneettipirneni
commented
Feb 6, 2023
suneettipirneni
marked this pull request as draft
February 6, 2023 18:00
vilgotf
reviewed
Feb 6, 2023
spring4175
self-requested a review
February 7, 2023 01:07
suneettipirneni
force-pushed
the
refactor/make-presence-guild-id-option
branch
from
February 8, 2023 02:40
2f3a610 to
4680cf8
Compare
suneettipirneni
marked this pull request as ready for review
February 8, 2023 02:40
vilgotf
self-requested a review
April 2, 2023 14:49
vilgotf
approved these changes
Apr 3, 2023
vilgotf
left a comment
vilgotf
left a comment
Member
There was a problem hiding this comment.
Looks good, as you've pointed out but I only now understand, the injection of presences' guild ID inside of the guild deserializer is necessary for the cache to work. Need to keep that in mind if we ever want to replace the manual guild deserializer with a derived one.
| @@ -34,7 +34,9 @@ impl UpdateCache for PresenceUpdate { | |||
|
|
|||
| let presence = CachedPresence::from_model(self.0.clone()); | |||
Member
There was a problem hiding this comment.
Let's move this allocation inside of the if statement
| @@ -1,188 +1,28 @@ | |||
| use serde::{Deserialize, Serialize}; | |||
|
|
|||
itohatweb
requested changes
Apr 25, 2023
| pub(crate) activities: Vec<Activity>, | ||
| pub(crate) client_status: ClientStatus, | ||
| pub(crate) guild_id: Id<GuildMarker>, | ||
| pub(crate) guild_id: Option<Id<GuildMarker>>, |
Member
There was a problem hiding this comment.
Actually I do not see a reason for this change. Except that the from_model would need to require another arg for the guild_id. But since the cache_presence method requires a guild_id anyways, this shouldn't be a huge problem, no?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As outlined in discord, this PR makes
Presence::guild_idan option. This is being done for various reasons.The first reason is for future-proofing against API changes.
Presenceisn't an explicitly defined structure in the API, but rather something that is the return value of a gateway event. Despite this, it does show up in other areas of the API. As of making this PR there isn't any situations where we receive a presence and have no knowledge of the guild id. However, as the field itself isn't always required to be sent with presences, the API may not always give twilightguild_ids with presences as it's not required to.Secondly, is to work towards removing manually deserializers. See #1364