Discord Name:
El Laggron#0260
GitHub Repository (Must be V3):
Description:
A repository of utility cogs providing tools for mods/admins for managing the server and the bot.
I’ll be starting a review on this today. Depending on how long it takes, there may be some delay with responses over the holidays.
Hope the season is treating you well, and I’ll be getting back to you with any feedback and concerns when I can.
Partial review here, I’ve intentionally not put comments on warnsystem here yet as I do not have time to right now, but would like to give you what feedback I do have for now in case you would like to get a start to fixing it before I’m finished with all of it.
Instantcmd:
instantcmd.py
L6: you aren’t using inspect. If this is for future stuff, comment it out.
L59-66: Appears to be dead code, though seems to have a purpose towards a future feature hinted at in your docs. Consider removing this or commenting it out if you intend to use it later.
L240-245: The issue you linked to in discord.py, Danny actually responded in how you could handle this. Should stop pointing to that issue and fix this. Users should not need to restart their bot to remove the listeners.
I’m still a little skeptical about people using this to add commands to their bot, rather than writing a command, or adding it through eval if intended to be short term, but it is owner only, so I don’t have a major issue with it.
RoleInvite:
errors.py
each error here should inherit from Exception
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.
Say:
say.py
L79: File download function:
I would highly advise not doing it in this fashion. The files do not need to be stored on disk at all, and adds an unneeded wget dependency.
There’s already a nice tool for this in Red.
from redbot.core.utils import tunnel
files = await tunnel.files_from_attach(some_message)
This is also designed specifically with the bot upload limit in mind, so you can safely use this out of the box with it.
Okay, here’s warnsystem. I’m not taking the time to reformat my notes on this.
Commit hash at time of review: d324799bfc165a8cf0c250b6e58d5e5dd053f53e
Warnsystem:
api.py:
L46: No. Absolutely do not do this. This is not an appropriate use of a global.
L47: Importing here is not the solution to your documentation problem.
L55: This method of pluralization is fundamentally incompatible with i18n. Drop it or the i18n.
L66-75:
The cascading divmods don’t need to be conditional,
and the dictionary access in this manner is an uneccessary performance hit.
You are translating the same strings over and over using the results as dictionary keys here.
L421-430, L836-847, (more in various locations):
Comparing values to translated results is not safe here.
L421-456 (Not repeating myself for each instance of this you do.):
Again, this is fundamentally incorrect for i18n.
Languages do not all follow this pattern, you cannot assume this can work.
You need positional, or ideally named parameters for format,
with the entire relevent string present.
See L:227-229 of warnsystem.py for an example of where you did this correctly
L909-910:
Don’t isinstance each channel in a guild. That’s expensive, and guild.text_channels exists.
errors.py:
(each)
Logging at exception creation, instead of at exception handling
is poor form, especially since you are using exceptions as part of control flow.
L132:
Class should at a minimum have a pass (linter would catch this)
warnsystem.py:
L49-70:
Missing attribution of borrowed code.
L92, various uses of this config value below:
Don’t recreate a setting which exists in the bot already. Use the bot settings.
Creating an override for the cog is fine here (this is not how you are using it),
but users should not have to set immunities, and heirarchy respecting per cog
L225:
Missing required parameter to function (linter would catch this.)
L323-341, (additional possible locations):
Sending embeds without this being tied to the purpose of your cog,
without respecting the setting for this
L426: (interacts with api.py:L520)
Something here needs to change. You should not be using isinstance on the result of a function
of yours to determine there was an issue.
It’s an expensive function call and should be used sparingly.
L795-802:
Consider changing this extended chaining of ternary statements to a lookup.
L1291:
Do not hook on_command_error when you are only interested in your own cog’s errors.
L1306:
construct this f-string better to avoid the replacement operation.
It’s an uneeded performance hit per message
Miscelaneous:
Never closing sentry session
You are using a lot of lambda statements where a ternary operator
would be better and more efficient (example: warnsystem L1235)
f-strings in logging is technically acceptable, but it would be better for
these to not be formatted at all and pass parameters for formatting since
a large portion of the time, many of these statements will never need to be evaluated
(debug level logging)
occasional f-string use without any interpolation (see L345 of warnsystem for an example of this)
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
Due to the size of the code reviewed and the scope of the issues needing fixing, I’m rejecting this application for now. Eligible for re-review after these are fixed, and no earlier than two weeks from now.