[APPROVED] Laggron's Dumb Cogs

Applications
5

Hi @El_Laggron, here’s my review. First I’m going to list the items that have not changed since your first two attempts.

  • Not properly disclosing that you are grabbing arbitrary messages on an error with sentry which are not guaranteed to be relevent to the issue You need to clarify in your enable asking exactly what user data you collect, and fix grabbing irrelevant messages.

Not fixed after initial review. This is really straight forward, when you first setup your cogs you must explain that your cogs have the option to collect x, y, and z to help you solve issues faster. To add onto this you’ve expanded this function to your other cogs since the initial reviews without extra declaration. Your logging also collects things that you do not care about, most of the data is benign from my testing but it must be explicitly stated what data you do collect through sentry.

RoleInvite

errors.py

  • each error here should inherit from Exception.

This has not been fixed since the first review.

roleinvite.py

  • On line 191, you check that the role is assignable by the bot due to hierarchy, but there needs to also be a check that the user should be able to give that role away. Right now, this is a privilege escalation issue. Someone could give themselves a higher role than they have by setting up an invite with this, quitting the server, and then using that invite.

Not fixed since first review.

Instantcmd

docs

  • [REJECTED (2) ] Laggron's Dumb Cogs The issue described in the docs here have not been completely fixed. Your example command says messag.send when it should be reaction.message.send. Furthermore your example for reaction add has no checks if the emoji is the emoji you want and replies to every emoji added including the one the bot added. You should provide a good example of how to use listeners that doesn’t lead to people running poor code when testing things out.

WarnSystem

warnsystem.py

  • Do not hook on_command_error when you are only interested in your own cog’s errors.

Not fixed since initial review. You should be using async def __error(self, ctx, error) to only collect errors within the cog.

These should have all been fixed before you attempted to re-apply and I’m disappointed that you have not fixed even the simple issues here.

===================================

Now onto other things that should be changed.

All Cogs

info.json

  • author is a list of strings now.
  • bot_version needs to be a list of integers.
    Reference.

cog.py

  • In your _set_log function you declare log as a global variable. This is unnecessary as your log will already be initialized when the cog is imported if you declare it in the header next to the imports. You can also then pass sentry to the cogs init alongside bot eliminating the need to have _set_log altogether.

Say

say.py

  • L38: self.translator is not used anywhere
  • L40: self.cache is not used anywhere
  • L344-346: Cleanup cache is unnecessary with using Tunnel
  • L257: This is overly complicated for use with language translation and could be better with on and off instead of enable/enabled and disable/disabled. (Optional)

WarnSystem

info.json

  • You should declare in your install_msg that this cog conflicts with core warnings cog by using the warn command.

warnsystem.py

  • L696: Should be [p]warnset showmod true not [p]warnset reinvite true

  • L669-680: Issue was brought up in DM’s after I was assigned to review this. Fixed in commit 34c1c83569f871404970b05bd418dc9d1f928b73

  • L1088-1095:
    You may consider using a lookup here instead of lambda as you have elsewhere in your cogs. (Optional)

  • L1260: See issue with L257 of say.py.

  • Re-invite on unban will only work if the user being banned shares another server with the bot and enabled DM’s from members on that shared server. It might be better to offer them an invite before banning which will only work once the ban is over.(Optional)

  • You format a timestamp in the embeds yourself, while they look nicer than discords will cause confusion when the timestamps don’t match up because the bot is running in another timezone. This can be solved by passing a timezone naive UTC datetime object to the embed.timestamp attribute.(Optional)