Fix some cache bugs and ensure member is always returned on delete calls #892
Conversation
davfsa
force-pushed
the
task/cache-bugs
branch
from
1b4d0f5
to
5d59eb5
Compare
| @@ -820,8 +820,6 @@ def _garbage_collect_member( | |||
| guild_record.members = None | |||
| self._remove_guild_record_if_empty(member.object.guild_id, guild_record) | |||
|
|
|||
| return member | |||
There was a problem hiding this comment.
You should probably just change this to return the object if it wasn't originally marked as has_been_deleted
There was a problem hiding this comment.
The reason I changed this to not return anything is because there is no need, as the information we return in the same as the one we take in. The cases we use this for atm don't even need what we return
There was a problem hiding this comment.
Clear and delete methods should only be returning objects which were actually marked as deleted not objects which were only being kept alive by references, the way you've changed the semantics still leads to erroneous behaviour and simply changing the behaviour of the garbage collect method will instruct both the delete and clear methods on what they should be saying was deleted and avoids a ton of other code changes
| @@ -1459,12 +1455,13 @@ def _garbage_collect_message( | |||
| *, | |||
| decrement: typing.Optional[int] = None, | |||
| override_ref: bool = False, | |||
| ) -> typing.Optional[cache_utility.RefCell[cache_utility.MessageData]]: | |||
| ) -> bool: | |||
There was a problem hiding this comment.
This change seems unnecessary and breaks the standard pattern for these methods
There was a problem hiding this comment.
The other methods currently only return None. I changed this since I dont think it makes sense to return the exact same thing that was sent in, considering how we are using this function atm
There was a problem hiding this comment.
The methods which were previously only returning None are doing this because they have no relevant delete and clear methods, they are not the same case
There was a problem hiding this comment.
think it makes sense to return the exact same thing that was sent in,
Look at how clear members was using it before
| @@ -1485,7 +1482,7 @@ def _garbage_collect_message( | |||
| if message.object.id in self._referenced_messages: | |||
There was a problem hiding this comment.
This should return None/False/whatever you feel like if it was already marked as deleted
There was a problem hiding this comment.
The uses for this function stay the same, so either its a bug I didnt fix or I did in fact miss something. I dont get the point for an extra return tho
There was a problem hiding this comment.
Clear and delete methods should only be returning objects which were actually marked as deleted not objects which were only being kept alive by references
davfsa
force-pushed
the
task/cache-bugs
branch
from
5d59eb5
to
f480e49
Compare
|
Will let @norinorin take care of this pr |
|
Sorry, wrong pr 😅 |
Checklist
noxand all the pipelines have passed.