[APPROVED] Vex-Cogs

Applications
#1

Discord Name: Vexed#3211

GitHub Repository (Must be V3):

Description: A collection of utility-based cogs.
I think everything should be pretty stable, but no promises :awesome:

#2
#3
#4
#5
#6
#7

Hi Vexed (Vexed#3211), thank you for your patience while I review your repo.

Commit hash at time of review - 07dc898e2678e13d03e9eaec46bda7961d047ded

All cogs

__init__.py

  • Reloading vex-cog-utils: This can break other cogs created by you if a user decides to only update one cog using the new verion and not another version since you’re reloading the module for all cogs using these utils. This issue can be more prevalent if a user decides to pin one of your cogs preventing it from updating even though you’ve updated them to use the latest version.

vex-cog-utils

  • This is included in the review as it is used by all your cogs.

sentry.py

  • The way the VexTelemetry cog is setup forces it to be loaded whenever your other cogs are loaded. As a result of this if all your other cogs are unloaded the VexTelemetry cog stays loaded. This should be addressed in some way. Either remove the cog the same way it was added when all your other cogs are unloaded or make the cog standalone to opt-in rather than including it on load of all your other cogs.

aliases

info.json

  • $schema is not a valid key for info.json (Optional)

anotherpingcog

anotherpingcog.py

  • Line 59: bot.loop.create_task is preferred over asyncio.create_task. (Optional)
  • Lines 318, 288, 459: You might consider using discord.PartialEmoji and discord.Colour as they have built in converters in discord.py to help cleanup a lot of your logic inside the commands. When PartialEmoji encounters a regular string though to solve that you can try adding a reaction to the ctx.message and if that fails you know it was not a valid emoji to use. (Optional)

info.json

  • $schema is not a valid key for info.json (Optional)

beautify

info.json

  • $schema is not a valid key for info.json (Optional)

utils.py

  • Line 116-121: You are stating here that input was not proper JSON format to begin with. The JSON format is entirely different from Python’s dictionary. In my opinion this line is unnecessary or at the very least should be clarified that the data provided was not in JSON format and was therefore changed to reflect that. (Optional)

__init__.py

betteruptime

abc.py

betteruptime.py

  • Lines 45-48: You can register all these variables in one command like (Optional):
self.config.register_global(version=1, cog_loaded=default, connected=default, first_load=None)

commands.py

  • Lines 188-191: This can be simplified into await self.config.clear(). (Optional)

info.json

  • $schema is not a valid key for info.json (Optional)

loop.py

  • Lines 156-169: This will not be 100% accurate all the time. The issue here is that an asyncio loop may not be exactly 60 seconds it could be 61 or 58 seconds between each interval. This is true no matter how much abstraction you put into the loop itself to try and mitigate it. This is because the loop is really a scheduled future object so the asyncio event loop is scheduling to run at that time but if anything blocks the event loop or the process decides this has less priority it won’t run exactly every 60 seconds on every machine. You might instead add seconds based on the timedelta between each loop iteration. (Optional)

cmdlog

cmdlog.py

  • Lines 435, 456, 478, 510: These should be guarded against file sizes larger than 8 MB (or the servers maximum upload size for all members) and attach_files permission.

info.json

  • $schema is not a valid key for info.json (Optional)
  • install_msg: “once you loaded” → “once loaded.” or “once you load it.” (Optional)
  • end_user_data_statement: “This cog does may” → “This cog may”. (Optional)

github

info.json

  • $schema is not a valid key for info.json (Optional)

madtranslate

info.json

  • $schema is not a valid key for info.json (Optional)

stattrack

commands.py

  • Lines 50, 52, 58, 60: This assert will raise an ugly error for users. You should properly guard this and explain or full the data appropriately. Asserts are not meant to check that your data is missing like this because a user is not going to understand why this errors and will just cause more confusion than an attribute or keyerror.
  • Lines 53, 61: You should check that you have permission to attach_files and the file size is not larger than the guilds upload limit.

info.json

  • $schema is not a valid key for info.json (Optional)

status

core/core.py

  • Line 58: You’re comment is right… It’s not too late though you can migrate to a new identifier and completely clear config on the old one on startup. I would recommend doing so.
  • Lines 72, 93: bot.loop.create_task is preferred over asyncio.create_task. (Optional)

info.json

  • $schema is not a valid key for info.json (Optional)

system

info.json

  • $schema is not a valid key for info.json (Optional)

tests

  • If this is not meant to be a cog that people install you should include the disabled key in the info.json so that downlaoder knows to avoid it and ignore it.

info.json

  • author is missing from info.json
  • install_msg is missing from info.json
  • end_user_data_statement is a requirement for data API and EUD compliance. (Optional)

timechannel

info.json

  • $schema is not a valid key for info.json (Optional)

wol

info.json

  • $schema is not a valid key for info.json (Optional)

The only major issues I have are how telemetry is handled on cog unload. Most other issues are superficial or require minor changes.

#8

Hey Trusty,

Thanks for such a detailed review. I have addressed most individual cog feedback, but not the reloading of vex-cog-utils, the VexTelemetry cog or telemetry on unload.

Sentry has been an interesting experiment and was fun to learn. However, on reflection, it does not pick up many errors at all and adds a decent chunk of code to each cog, even the simple ones. Therefore I think it’s best to remove it completely, especially as my cogs move to a less technical user base after approval, of which I feel it is more morally questionable as to whether telemetry (even opt-in) is acceptable.

Publishing vex-cog-utils as a PyPi package was also a nice thing to learn. However, I had not considered your point about different cogs on different versions. Hot reloading is in a way required as it is a better solution than having to do a bot restart each time. I think that the best way to do this would be to do utils PCX Cogs-style where it’s a single file (or folder?) which is copied to each cog - which I would want to automate, so that’s something to learn.

I have decided to keep the $schema key in my info.json.

Per-cog notes:

  • I will not be changing how the BetterUptime loop is scheduled. A while ago, the method you described was used but to simplify both the loop code and code in other places, I decided to keep it in increments of 60. I’m not too concerned about it being a few seconds out, and I haven’t noticed much of an issue with this on my bots. Thank you for bringing this to my attention with such detail, I’ll try and keep it in the back of my mind.
  • Thank you for mentioning file size limits. This isn’t something I’ve thought about before.
  • For the asserts in StatTrack, I have removed them for now and long-term I plan to ensure it is defined by using an async setup function to do the initial data read before add_cog, while as at the moment a task is created in the sync init to define them which could potentially happen after a command is invoked.
  • Tests is indeed not meant to be a cog, so I assume I can ignore the missing keys warning.

So, this is mainly me noting down where I’m at and not necessarily something you need to respond to. Please do not approve me until I have removed Sentry, which I think will just leave reloading of Vex-Cog-Utils which I will fix as described above later on. Depending on how important you think reload issues with Vex-Cog-Utils are, I would be happy to be approved once Sentry has been removed, though I am also equally happy to wait.

Thank you again for taking time to review my cogs.

#9

Hey Trusty,

The feedback which I hadn’t previously addressed as noted in the previous post has now been fixed in the last few commits and if there’s anything else please do let me know.

#10

Hi Vexed, thanks for making those changes. I am still not 100% on the asserts in the code, if you want to prevent a command from executing until the init is fully complete you might look at creating an asyncio.Event() that is set only once all initialization is complete rather than potentially throwing ugly errors at people who will not understand why if ctx.guild is something completely different than a discord.Guild object and is also not None. Regardless I will be marking this as approved.

#11