[Bank] Dispatch events for account events #5325
Conversation
github-actions
bot
added
Category: Core - API - Bank
github-actions
bot
added
Category: Docs - Other
github-actions
bot
added
Category: Cogs - Economy
redbot/core/bank.py
Outdated
| self.recipient_new_balance = recipient_new_balance | ||
|
|
||
|
|
||
| class BankPruneInformation: |
There was a problem hiding this comment.
The type annotation for pruned_users should be Union[List[int], Dict[str, List[int]]] instead of Union(List[int], Dict[str, List[int]])
|
Would it be possible it to make these payload classes into NamedTuples instead? That should be a free performance improvement since these classes don't need to be modified after they're created. |
| .. py:method:: red_bank_set_balance(payload: redbot.core.bank.BankSetBalanceInformation) | ||
| Dispatched when a user has their balance changed. |
There was a problem hiding this comment.
The documented events that dispatch custom objects should link to those objects in some way. It might make sense to use a similar format as the autogenerated API descriptions for methods, which have a "parameters" section describing each of the parameters in detail.
| return deco | ||
|
|
||
|
|
||
| class BankTransferInformation(NamedTuple): |
There was a problem hiding this comment.
These objects should be added to __all__, since they are part of the public API. They also need to be documented, which may or may not happen automatically after doing so.
| """ | ||
|
|
||
| print(f"{guild} | {user_id}") |
| await group.name.set(member.display_name) | ||
|
|
||
| payload = BankSetBalanceInformation( | ||
| member.id, (0 if _cache_is_global else guild.id), old_balance, amount |
There was a problem hiding this comment.
It's more typical for guild like attributes to be Optional and use None as the value to represent non-guild actions (IE, message.guild). If it is consistently possible to provide the full user/guild object rather than an ID, I also think that would be preferred.
|
|
||
| .. py:method:: red_bank_wipe(guild_id: int) | ||
| Dispatched when the Bank gets wiped. :code:`guild_id` will be the ID of the Guild that was wiped, 0 if all Guilds were wiped (Bank is Local), or -1 if all Users were wiped (Bank is Global) |
There was a problem hiding this comment.
Having both 0 and -1 as special values seems a bit smelly. Is there a meaningful difference for the CC to account for that makes differentiating between wiping guilds or users something that needs to happen? If so, I think it would be neater to just have a second argument, whether or not the bank was global at the time of wiping.
I would say the CC can just check if the bank is global or not to get the same value, but that only applies for [p]bankset reset. [p]bankset toggleglobal also implicitly results in this being called, since it clears the bank during the process of swapping scope. Trying to test for it at that time would likely result in an incorrect value due to race conditions.
| if user_id in bank_data: | ||
| del bank_data[user_id] | ||
|
|
||
| payload = BankPruneInformation(scope, tmp) |
There was a problem hiding this comment.
tmp is only defined when user_id is None. This errors if a particular user is pruned.
(I do think the conditionally defined variables in this function are smelly, making this sort of error very easy to make)
| if guild is None and user_id is None: | ||
| scope = 1 # prune global | ||
| if guild is not None and user_id is None: | ||
| scope = 2 # prune server | ||
| else: | ||
| scope = 3 # prune user |
There was a problem hiding this comment.
Forgot to hit submit on the comment I was writing here....
Doing it like this loses the ability to know what guild was being pruned. Unless there is some reason to differentiate between specifically pruning a member and a single member happening to be the only invalid member from a bulk prune, the signature of this event object would likely be better as:
guild: Optional[discord.Guild], pruned_user_ids: List[int]
6 participants
Adds some event dispatches to Bank
red_bank_set_global
red_bank_set_balance
red_bank_transfer_credits
red_bank_wipe
red_bank_prune_accounts
Resolves #5141