Skip to content

Conversation

mkrasnitski
Copy link
Collaborator

@mkrasnitski mkrasnitski commented Nov 26, 2024

The gateway was generating extra FullEvents by directly inspecting the contents of the cache, however this functionality can be incorporated as part of the CacheUpdate implementation for those events. The main cache fields are also private now. The temp_cache fields could be made private too, but I'm unsure if it's worth it.

@github-actions github-actions bot added cache Related to the `cache`-feature. gateway Related to the `gateway` module. labels Nov 26, 2024
@mkrasnitski
Copy link
Collaborator Author

Blocked on #3053 for now. Don't feel like fixing code that's gonna get removed anyways.

@mkrasnitski mkrasnitski force-pushed the cache-update branch 3 times, most recently from 7547066 to 2dd5053 Compare December 2, 2024 17:13
@mkrasnitski mkrasnitski force-pushed the cache-update branch 2 times, most recently from e758fa6 to 9f19cbd Compare December 10, 2024 04:34
@mkrasnitski mkrasnitski force-pushed the cache-update branch 2 times, most recently from 3cfd9a1 to faa1aad Compare January 17, 2025 01:44
@GnomedDev
Copy link
Member

Since this is blocked on another PR, I am changing to draft.

@mkrasnitski mkrasnitski marked this pull request as ready for review July 25, 2025 15:43
Copy link
Member

@GnomedDev GnomedDev left a comment

Choose a reason for hiding this comment

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

LGTM

},
Event::GuildDelete(mut event) => {
let full = if_cache!(event.update(cache));
let full = if_cache!(update_cache!(cache, event));
Copy link
Member

Choose a reason for hiding this comment

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

Why change this? I'm fine with merging as-is, just curious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Simply because update_cache! is used elsewhere in the file to do the same thing. Figured I'd make it consistent.

@GnomedDev GnomedDev merged commit e47f38c into serenity-rs:next Jul 26, 2025
23 of 24 checks passed
@mkrasnitski mkrasnitski deleted the cache-update branch July 26, 2025 22:20
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Jul 28, 2025
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Jul 28, 2025
BossFlea pushed a commit to BossFlea/serenity that referenced this pull request Sep 16, 2025
BossFlea added a commit to BossFlea/serenity that referenced this pull request Sep 16, 2025
…y-rs#3066)" to restore compatibility with poise

This reverts commit bf79aa9.
BossFlea added a commit to BossFlea/serenity that referenced this pull request Sep 20, 2025
This reverts commits:
e5a4c30 "Remove ArgumentConvert trait and implementations (serenity-rs#3053)"
bf79aa9 "Fix CacheUpdate implementations for extra FullEvents (serenity-rs#3066)"
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Oct 7, 2025
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Oct 7, 2025
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Oct 7, 2025
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. gateway Related to the `gateway` module. model Related to the `model` module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants