-
Notifications
You must be signed in to change notification settings - Fork 20
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
Don't update message in cache by reference #128
base: master
Are you sure you want to change the base?
Conversation
# Update the messages | ||
if exists: | ||
messages[msg.id] = newMsg | ||
s.checkAndCall(MessageUpdate, newMsg, option(oldMsg), exists) |
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.
Should newMsg
use the full data? I kept it like this since the old code didn't update the message object if nothing existed in cache but should it instead also try to add what data it can to the new message?
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.
It should always contain the full data just like the old one. if old message doesnt exist in cache then the oldMsg param should be none
as always. The new message should have the data equivalent to what was in the raw json data, which I seem to have made a mistake on and rereading the old code it only contains id and channel_id if it doesnt exist in cache. It should be msg = newMessage(data)
.
I'm assuming that on the discord docs it used to mention that message data in the event did not contain the whole message data before, so hence why it might have led to that line of code. But seems now currently that's no longer the case after checking the docs.
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.
Nice, that makes it a bit simplier. Now it creates a new message and updates the cache
Always update the cache
@@ -655,7 +655,9 @@ proc newInviteMetadata*(data: JsonNode): InviteMetadata = | |||
result = data.`$`.fromJson(InviteMetadata) | |||
|
|||
proc updateMessage*(m: Message, data: JsonNode): Message = |
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.
This is unused, could be deleted maybe?
Fixes #127
Since
Message
is a reference, when we update the message in the table it was updated the message. Fix is to copy the message and then add the new message into cache (Think this might've been my fault when trying to move away from deepcopy?)Testing evidence that it can message in cache and not in cache
Using this test code