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

Add cached messages to channel_delete event handler #2194

Merged
merged 5 commits into from Oct 2, 2022

Conversation

emmaboecker
Copy link
Contributor

The Reason I'm making this PR is issue #2180

If there is a better way to do this I'd be happy to implement that instead (especially if it doesn't introduce a breaking change).

Updating the cache after the event is handled is not possible without having to clone event.channel which I didn't wanna do.

Closes #2180

@github-actions github-actions bot added the client Related to the `client` module. label Sep 30, 2022
@kangalio
Copy link
Collaborator

kangalio commented Oct 1, 2022

CI fails if the cache is disabled. The code should only get the messages from cache if it is enabled.

For consistency, this should happen in the CacheUpdate implementation for ChannelDeleteEvent, like already done for member/guild/role/thread deletes. That also takes care of only running the code when cache is enabled

@emmaboecker
Copy link
Contributor Author

I'm confused, do I have to do something or is this alright now?

@mkrasnitski
Copy link
Collaborator

You'll have to rebase your PR on top of next. I recommend doing git remote add upstream git@github.com:serenity-rs/serenity.git and git fetch upstream next if you haven't already, then rebase or cherry-pick your commits on top of it, then do a force push to your origin.

@emmaboecker emmaboecker closed this Oct 2, 2022
@github-actions github-actions bot removed cache Related to the `cache`-feature. client Related to the `client` module. labels Oct 2, 2022
@emmaboecker emmaboecker reopened this Oct 2, 2022
@github-actions github-actions bot added cache Related to the `cache`-feature. client Related to the `client` module. labels Oct 2, 2022
src/cache/event.rs Outdated Show resolved Hide resolved
Co-authored-by: Michael Krasnitski <42564254+mkrasnitski@users.noreply.github.com>
Copy link
Collaborator

@kangalio kangalio left a comment

Choose a reason for hiding this comment

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

LGTM. If a maintainer could run CI that'd be great

@emmaboecker
Copy link
Contributor Author

I always forget to format with nightly haha

src/cache/event.rs Outdated Show resolved Hide resolved
Co-authored-by: Alex M. M. <acdenissk69@gmail.com>
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Feb 28, 2023
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request May 18, 2023
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request May 30, 2023
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Sep 21, 2023
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Oct 17, 2023
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Oct 24, 2023
arqunis pushed a commit to arqunis/serenity that referenced this pull request Oct 24, 2023
arqunis pushed a commit to arqunis/serenity that referenced this pull request Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cache Related to the `cache`-feature. client Related to the `client` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants