Description: Threadweaver is a cog that allows users (above a specified role rank) to create temporary semi-private thread channels by reacting to messages with the emoji. Other users can join the thread by adding their reactions.
After a specified time (default: 1 Day) these thread channels are merged and consolidated into a thread_archive channel, where they won’t clutter up the side-bar for Admins or Users.
Hi zalo#5389, thank you for your patience while I review your repo.
Commit hash at time of review - f45a4e8877c5d88f3e1ee5d02c8681377ab34c57
threadweaver
info.json
end_user_data_statement is missing from info.json
end_user_data_statement is a requirement for data API and EUD compliance. I also recommend you include
async def red_delete_data_for_user(self, **kwargs):
"""
Nothing to delete
"""
return
Inside threadweaver.py so that users can feel secure if they choose to delete their data on the bot knowing that nothing is saved. Without this function if a user requests their data be deleted the bot will return saying that it’s not sure if the data has been deleted and might cause concern but since you’re not storing any user information in this cog it’s optional to remove that warning. (Optional)
threadweaver.py
Lines 58, 119, 145, 301, 364, 365, 418, : The correct typhint for this is List[Object] or typing.List[Object] using from typing import List or import typing. For example on line 58 List[str] and line 145 List[Message] (Optional)
Lines 96-125: It took me a bit to understand how this was functioning. While I can appreciate the attempt to reduce the number of required commands this is, in my opinion, needlessly complex and unintuitive. A user would have to know exactly what you named the config values and what they represent. Part of the reason a lot of cog creators opt not to handle things this way is they can explain more information in the docstrings for the command about exactly what it is doing. Furthermore this limits usability where you should consider saving ID’s rather than names for things like channels categories and roles. They’re far easier to find later on and prevent having to verify the server structure all the time. If a channel, role, or category ID can’t be found then it’s gone and you can stop operation. Saving names mean if any change to the name of something for whatever reason it will fail or in your case re-create with the saved name which is unintuitive. The other nice thing about using multiple commands for each setting you want to change is typehints can convert into the explicit object you want to store. For example: async def threadweaver_update_role(self, ctx, *, role: discord.Role): when that command is run the user does not have to worry about accidentally mentioning the role cause they can just type the (case sensitive) name of the role in the command to change it. If you then store the ID for that later the user may change the name however they want without breaking everything. My recommended solution is to split this up into multiple commands perhaps under a new command group and change the stored values to ID’s rather than names. At the bare minimum your doc strings here need to state what all settings are available you want to let users change and a warning that changing the names without changing in the command could cause things to re-create themselves.
Lines 143-178: You need to be ensuring the bot has embed links permission somewhere before posting otherwise it will error without warning to the user. It’s also worth noting since you’re using embeds and are combining multiple messages into one embed you actually have 2048 character limit on embeds description field and up to 6000 total characters allowed in an embed with multiple fields so you can likely reduce the total number of sent messages for archiving. It also might be worthwhile restructuring this to be an iterative process on line 145 going through the messages as the bot gets them rather than pulling them all first and then posting. Reading message history is an expesinve API operation and pulling 5000 messages means 50 API calls which you can be doing other stuff while waiting for discord to let you pull the next 100 such as posting the messages you’ve already collected.
Lines 151, 193, 235, 355, 394, 437, 483: You keep manually building a member mention. While this isn’t necessarily bad it signifies to me that you may not be aware that member.mention exists which builds the mention for you without doing your own string manipulation.
Line 136: Lesser known but mention regex for users actually sometimes include a ! so your regex should be <@!?[0-9]{17,}> the final portion will also never (in practice) have 0 numbers for an actual user mention and the minimum amount of numbers a discord snowflake can be is 17 so we check if there’s 17 or more numbers between 0 and 9.
commands rename_thread_command, delete_all_threads_command, and archive_thread_command are missing docstrings that should explain what the command does. The description you have set doesn’t really do them justice as you start to break pythons line limit recommendations which is why we pull the info from doc strings for the help command.
Line 368: You can get a members top role via member.top_role and you should generally compare the roles themselves in hierarchy rather than position. There are times when positions of roles are identical that can lead to issues where if you compare the roles themselves if the position is the same it will compare the age of the role instead.
Lines 205, 213, 256, 273, and 405: I like that you’re using actual logging rather than print but info for this is unnecessary for most end-users I recommend changing them to debug and running your development bot with the debug flag so you can see them and understand what’s going on. (Optional)
Line 372-376: Your logic here to limit users creating threads should be complete before I can accept this cog. Having the potential for any user to create/delete channels at will can very quickly lead to API spam which can get ones bot temporarily banned from discord. The comment on line 45 also suggests that this is enabled for anyone right out of the gate once the cog is loaded and someone reacts with . There absolutely needs protection preventing this from accidentally happening on servers where it’s not wanted. If someone adds a bot (or the cog to their bot) and it creates a channel category and channel automatically people will either kick the bot or report it to discord.
on_raw_reaction_add and on_raw_reaction_remove should both respect Red’s cog_disabled_in_guild setting so that the cog can be disabled by default by the bot owner and enabled via server admins if wanted. These listeners should also have a lot more in-place to prevent abuse as explained above since immediately loading the cog allows anyone with any permission on a server where the bot can do-so generate the channels which is considered unexpected behavior when done via reactions. I would recommend ensuring at minimum users who have manage_channels permission unless explicitly overruled with another command.
General comments: This is a great start for a niche use-case cog. I believe I’ve outlined most of the major issues your users will encounter trying to use this. Some other things I wanted to comment on is the lack of using command groups which can simplify your command structure by having a top level [p]threadweaver command that then lists things like [p]threadweaver settings, etc. which will greatly improve the usability of this cog without complicating commands with unnecessary dashes in the command names but that’s personal choice I only mention here since it seems like you may not have explored that idea yet and it doesn’t affect this review.
Hello, it’s been 25 days since I reviewed this application and have not seen any progress. I attempted to reach out on discord but I can’t seem to find any record of you in https://discord.gg/red . As a result of being unable to reach out directly I will be rejecting this application until such time the listed issues have been addressed and no less than 1 week from today. I also encourage you to join the Red discord server or update your discord name in this post if it’s different and you are in the server.
emoji. Other users can join the thread by adding their reactions.