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

fix(interactions): get info from cache first if available #724

Merged
merged 5 commits into from
Jul 16, 2022

Conversation

Soheab
Copy link
Contributor

@Soheab Soheab commented Jul 1, 2022

Summary

Fixes #605

This PR changes the assignment of the following attributes for Interaction to try cache before constructing the object: .message, .user

This is needed because fields like status are not sent in the Interaction member payload but are usually cached.

Preview: image
I don't know how to test .message and User for .user but the correct object is returned.

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
    • I have run task pyright and fixed the relevant issues.
  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

@netlify
Copy link

netlify bot commented Jul 1, 2022

Deploy Preview for nextcord-gh-action ready!

Name Link
🔨 Latest commit bcb273b
🔍 Latest deploy log https://app.netlify.com/sites/nextcord-gh-action/deploys/62bf65b5e8ca4c00086a6999
😎 Deploy Preview https://deploy-preview-724--nextcord-gh-action.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@DenverCoder1
Copy link
Collaborator

DenverCoder1 commented Jul 1, 2022

If not all intents are enabled, there may be some events that don't update the cached message I think.

Is it possible the interaction message will contain data that is newer/more accurate than the cache?

Possibly they may need to be merged together?

Copy link
Collaborator

@EmmmaTech EmmmaTech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

nextcord/interactions.py Show resolved Hide resolved
Copy link
Collaborator

@DenverCoder1 DenverCoder1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know what you think of these suggestions. This should use the cache, but also update any additional fields provided by the interaction data.

nextcord/interactions.py Show resolved Hide resolved
nextcord/interactions.py Show resolved Hide resolved
nextcord/interactions.py Show resolved Hide resolved
@Soheab
Copy link
Contributor Author

Soheab commented Jul 11, 2022

If not all intents are enabled, there may be some events that don't update the cached message I think.

Is it possible the interaction message will contain data that is newer/more accurate than the cache?

Possibly they may need to be merged together?

If not all intents are enabled, there may be some events that don't update the cached message I think.

Is it possible the interaction message will contain data that is newer/more accurate than the cache?

Possibly they may need to be merged together?

keep forgetting to respond, sorry
But no, I haven't experienced a situation where the Interaction Message was missing information or was out of date, mainly because it's not really used since Interaction has most of the stuff

I just did it for the sake of "why not".

If not all intents are enabled, there may be some events that don't update the cached message I think.

Same as above, I don't know but if that's true but that doesn't sound good and a potential dataloss.


Response to the suggestions:

They look good to me, will of course require testing. Will do that when I can, thank you :)

@DenverCoder1
Copy link
Collaborator

But no, I haven't experienced a situation where the Interaction Message was missing information or was out of date, mainly because it's not really used since Interaction has most of the stuff

I believe you are correct since if the member/message is cached at all, it should always update in the corresponding update/edit event, so just using the item from the cache should be enough.

@ooliver1 ooliver1 added t: enhancement Type: enhancement - new feature or request p: low Priority: low - not important to be worked on s: awaiting review Status: the issue or PR is awaiting reviews 2.1 The issue/PR should go for the 2.1 release labels Jul 16, 2022
@ooliver1 ooliver1 added this to the 2.1 milestone Jul 16, 2022
@ooliver1 ooliver1 added t: bug Type: bug - something isn't working and removed t: enhancement Type: enhancement - new feature or request labels Jul 16, 2022
@ooliver1 ooliver1 changed the title fix: try getting message, member and user from cache first for Interaction fix(interactions): get info from cache first if available Jul 16, 2022
@ooliver1 ooliver1 merged commit afdd7e5 into nextcord:master Jul 16, 2022
@ooliver1 ooliver1 removed the s: awaiting review Status: the issue or PR is awaiting reviews label Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.1 The issue/PR should go for the 2.1 release p: low Priority: low - not important to be worked on t: bug Type: bug - something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Status not displaying properly
4 participants