reorder db interactions and add types to starboard.py #123
Conversation
|
Also, added a fallback that watches for message deletions in the starboard channel, and adds blacklist entries for them. |
There was a problem hiding this comment.
I am still uncertain of a few things, so take all comments with a pinch of salt, I think it would be really nice to have the wiki, as there are a few terms I keep encountering that would be nice to define ("starboard update" in particular). It would also be nice to have a description of the of the starboard database and what None means for certain entries.
The typing helps quite a bit for understanding the overall program structure, so a win for Pyright. I haven't full gotten my head around the overall flow of adding starboarded messages, but from what I understand, the _starboard_db_add and _starboard_db_finish methods seem to tighten the race issue, though two almost simultaneous calls to on_raw_reaction_add could still cause _process_sb_updates to run twice. Also the fallback for deleted messages is great.
I'm also going to piggyback on this review and ask a list of questions about starboard that weren't changed by this pull request, but would help me understand the code:
- Why are runtime errors used and what would be the symptoms if one of them occurred?
- Why is there
time.sleep(5)incleanup_starboard(in the latest version, line 130)?
| class StarboardMsgPair(object): | ||
| def __init__( | ||
| self, | ||
| recv: Union[discord.Message, None], |
There was a problem hiding this comment.
Personally, I think Optional[discord.Message] is a little bit more clear, but I'm also happy as is.
| self, | ||
| recv: Union[discord.Message, None], | ||
| sent: Union[discord.Message, None], | ||
| recv_channel: Union[ |
There was a problem hiding this comment.
Similarly, I find that Optional[discord.abc.GuildChannel | discord.Thread | discord.abc.PrivateChannel] is more readable, but again, not too fussed.
| class BlacklistedMessageError(Exception): | ||
| # always caught. used to differentiate "starboard message doesn't exist" and "starboard message is blacklisted" | ||
| pass | ||
| class StarboardMsgPair(object): |
There was a problem hiding this comment.
I think it would be nice to have a docstring to explain the difference between recv and sent. It took me some time to understand what they meant.
| ) | ||
| else: | ||
| if message_id == 1076779482637144105: | ||
| # This is Isaac's initial-starboard message. I know, IDs are bad. BUT | ||
| # consider that this doesn't cause any of the usual ID-related issues | ||
| # like breaking lookups in other servers. | ||
| return | ||
| return StarboardMsgPair(None, None, None, True) |
There was a problem hiding this comment.
I think using keywords for the arguments here would be useful. Similarly when this pattern reoccurs.
| self, payload: discord.RawMessageDeleteEvent | ||
| ) -> None: | ||
| """ | ||
| Fallback that blacklists messages whenever a starboard message is deleted. See documentation for context blacklist commands. |
|
|
||
| def dangerous_recv(self) -> discord.Message: |
There was a problem hiding this comment.
I didn't understand the dangerous commands on first run through; a doc string would be nice but not necessary.
| db_session.close() | ||
|
|
||
| def _starboard_db_finish(self, recv: int, recv_location: int, sent: int) -> None: | ||
| """Finalises a starboard DB entry. Finishes the job that _starboard_db_add starts - it adds the sent-message-id.""" |
There was a problem hiding this comment.
This docstring was very helpful
| @@ -232,13 +275,37 @@ def _rm_big_ratelimit(self, id: int) -> None: | |||
| """Callback to remove a message from the big-ratelimited list""" | |||
| self.big_blocked_messages.remove(id) | |||
|
|
|||
| def _starboard_db_add(self, recv: int, recv_location: int, sent: int) -> None: | |||
| def _starboard_db_add(self, recv: int, recv_location: int) -> None: | |||
| """Creates a starboard DB entry. Only called from _process_updates when the messages are not None, so doesn't | |||
There was a problem hiding this comment.
Potentially mention that _starboard_db_finish should be run at some point after this method.
JamesDearlove
changed the title
@peclarke prompted a galaxy-brain idea - by reordering the DB interactions (and adding a fake-blacklist-entry to the DB, temporarily), we should be able to fix the starboard doubleposting.
This is a "oh shit cool idea" PR, and as such isn't tested and I haven't thought about side effects. I'm just getting a quick thing out there so I don't forget in the middle of exam fog!
Potential problems/other notes: