Alright, I’m pretty sure everything is fixed now, referring to all 3 reviews. I listed all the issues and associated the commit that fixes it, so we can be sure that, this time, I’m not letting issues unresolved…
First review
InstantCommands
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.
RoleInvite
errors.py
each error here should inherit from Exception
Fixed on time, overwritten by another branch and fixed back
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.
Fixed on time, overwritten by another branch and fixed back
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.
WarnSystem
The first review was closed after the WarnSystem review, so all the changes were done after.
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.
That was actually already on line 31, but I added it a second time after first review
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
We had a discussion in DM, this setting is not included on Red for now.
“sigh, yeah go ahead and ignore that one for now, but just keep in mind that may become a bot global setting like the automod immunities later (it was supposed to have, but it’s too late to happen for 3.0.0 now, and I didn’t notice that change hadn’t happened.”
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
Still don’t get what was meant here.
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.
This actually prevents logging other errors:
if not ctx.command.cog_name == self.__class__.__name__:
return
L1306:
construct this f-string better to avoid the replacement operation.
It’s an uneeded performance hit per message
Fixed after first review but Sentry is now removed from all cogs.
Miscelaneous
Never closing sentry session
Sentry is removed.
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)
Still makes it easier for me, isn’t translated and doesn’t reduce performances…
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
Sentry completly removed.
Second review
Two issues were mentionned in that review:
- the outdated docs after the breaking changes, fixed
- the listeners being added multiple times upon cog reloads, fixed
Third review
Before referring all commits fixing the problems, just one thing to tell: I completly removed Sentry since it’s causing more problems than it solves, especially with sentry-sdk which is way more complicated for a cog.
Things that were not fixed
Sentry
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.
Sentry is removed.
RoleInvite
each error here should inherit from Exception
As I said above, fixed on time, overwritten by another branch and fixed back.
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.
Same here, fixed on time, overwritten by another branch and fixed back.
InstantCmd
The issue described in the docs here have not been completely fixed. Your example command says
messag.sendwhen it should bereaction.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
Do not hook on_command_error when you are only interested in your own cog’s errors.
This has never been a problem since I use this inside the function, filtering errors that are not mine. Also, converting this to __error somehow caused a lot of issues, so I’m keeping this block of code, this only log errors from the current cog.
if not ctx.command.cog_name == self.__class__.__name__:
return
New issues
info.json
author is a list of strings now.
bot_version needs to be a list of integers.
cog.py
In your
_set_logfunction 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_logaltogether.
Sentry removed.
Say
- 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
onandoffinstead of enable/enabled and disable/disabled. (Optional)
Sentry removed, this includes this part.
WarnSystem
You should declare in your install_msg that this cog conflicts with core warnings cog by using the warn command.
L696: Should be
[p]warnset showmod truenot[p]warnset reinvite true
L669-680: Issue was brought up in DM’s after I was assigned to review this. Fixed in commit 34c1c83569f871404970b05bd418dc9d1f928b73
You do the work for me 
L1088-1095:
You may consider using a lookup here instead of lambda as you have elsewhere in your cogs. (Optional)
Technically it’s already a lookup, it uses dict.get.
L1260: See issue with L257 of say.py.
Sentry removed.
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)
It’s a good idea, I’m adding this on my todo list for the next version, since this is not required right now.
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)
Same here.