[CLOSED] OofCogs

Applications
#1

Discord Name: OofChair#0001

GitHub Repository (Must be V3): https://github.com/OofChair/OofCogs

Description: A repo with a couple of cog ideas that have come to my mind. Please let me know if I need to change anything. Thanks!

#2
#3

Hi OofChair#0001, thank you for your patience while I review your repo.

Commit hash at time of review - 3fcb56e7ed5c48330e5c02bcb27bb41f1ee503b7

advancedeconomy

advancedeconomy.py

  • Line 57: You’d be better off using a default of 0 rather than the time the cog was loaded.
  • Lines 99, 109, 120: All these commands exist under the Bank cog which do not conflict with your economy replacement. Duplicating these commands seems entirely unnecessary especially since you replaced the verbose feedback to a single tick which doesn’t provide a lot of useful information to users using it.
  • Line 60: Registering the cooldown on members means that users will have individual cooldowns per server bypassing your other restrictions. I would recommend using register_user instead to only have one cooldown time per user.
  • Line 136: You may want to guard against negative cooldown times. (Optional)
  • Line 148: next_payday in config according to your registered defaults will never be None. (Optional)
  • Line 192: You never use the variable credit_name. (Optional)
  • Lines 57, 151, 155, 174: You should consider using a timezone aware datetime object and save that instead. datetime.datetime.now() assumes local system time while discord timestamps require a UTC timestamp so I recommend datetime.datetime.now(datetime.timezone.utc) just to ensure all stored timestamps are UTC specifically and there’s no confusion on differing systems. (Optional)
  • Line 214: Variable e is assigned but never used. If you’re going to assign a variable in an exception you should do something with it or ignore it completely like except ValueError:

info.json

  • Inside your install message you need to explain to users that this forces the bots economy to be global.

diaccents

diaccents.py

  • Line 39: while this doesn’t do anything significant it’s not exactly the right approach, you should just return or pass. Calling super() is not the recommended way to handle no data to delete. (Optional)

info.json

  • requirements: Missing required library dcl.

invitetracker

invitetracker.py

  • Line 46: while this doesn’t do anything significant it’s not exactly the right approach, you should just return or pass. Calling super() is not the recommended way to handle no data to delete. (Optional)
  • Line 50: You should be checking that the bot has manage_guild permissions rather than attempting and failing to grab the invites. This method makes the call regardless eating up the bots rate limits for gathering this information.
  • Line 64: bot_in_a_guild() check only checks that the bot is in at least 1 guild before executing the command. You should be checking that the context the command is sent in is a guild done with @commands.guild_only() otherwise all your commands will fail in DM.
  • Line 83-85: You should be checking here that you actually have permission to send messages in the channel provided along with embed links permission.
  • Lines 85, 96, 110, 128, and 152: These should be outside the typing context (read: unindented) so that the bot does not continue typing after sending the message. Considering these are very short small commands you may choose to replace async with ctx.typing() with await ctx.trigger_typing() instead which will automatically end after the following ctx.send().
  • Line 144: This will fail if the bot is missing manage_guild permissions.
  • Lines 168 and 200: You should be checking that you have manage_guild permissions rather than trying to get the invites as this could heavily rate limit the bot with a ton of joins simultaneously. I would also recommend setting up a listener for invites created and deleted so that you don’t need to pull the guild invite list every time a user joins and rely on your own internal cache from that. Right now you cannot track 1 time use invite codes that are used immediately as the information will be gone from guild.invites before you have a chance to pull it.
  • Lines 181-185 and 213-217: Di you really intend to continue sending a log message if the bot is missing manage_guild permissions?

redditpic

info.json

  • You should not put aiohttp into your requirements, Red already provides a version of aiohttp and you could attempt to install an incompatible version for people with this.

redditpic.py

  • Line 36: You should use self.bot.loop.create_task(self.session.close()) here, while run_until_complete implicitly does this for coroutines like this you should be explicit that you’re creating a task here. (Optional)
  • Line 42: while this doesn’t do anything significant it’s not exactly the right approach, you should just return or pass. Calling super() is not the recommended way to handle no data to delete. (Optional)
  • Linea 73-98: You need to guard this command against sending NSFW content to non-NSFW channels.

serverping

__init__.py

  • Missing __red_end_user_data_statement__ which is required to be fully compliant with end user data declaration. (Optional)

sql

info.json

  • min_bot_version should be 3.4.6 in order to utilize ctx.reply

sql.py

  • Lines 98 and 143: Using a tick() alongside a text reply is somewhat redundant. (Optional)

sxcu

info.json

  • $schema is not a valid key for info.json (Optional)
  • end_user_data_statement is a requirement for data API and EUD compliance. (Optional)

README.md

__init__.py

  • Missing __red_end_user_data_statement__ which is required to be fully compliant with end user data declaration. (Optional)
#4

Hi Trusty! Thanks for taking the time to review my repo - all changes should be made in commit 669346c458b443c10042a696c900de7cb0278e1f, let me know if I need to fix anything else :slight_smile:

#5

Hi OofChair#0001, there’s still some things that need to be addressed.

Commit hash at time of second review - 872f6cca52f2860d90b936ed1252e1e35d406fc4

invitetracker

invitetracker.py

  • Line 50: You never assign me as an attribute to the cog so this will error and you never know. You must have misunderstood how I meant for you to check this from my previous review.
for guild in self.bot.guilds:
    if guild.me.guild_permissions.manage_guild:
        ...

You completely removed your loop looking at guilds and the previous commit you tried to pass ctx which was also incorrect. You can’t just pass things to a function without actually having their reference.

  • Line 58: You really shouldn’t be raising this but even if you did you would never know because of how you created this task. Tasks created like this one is send the error to its own context manager contained within the task itself. You spin up and tell the bot to create the task but never check back to see that it has an error or that it even completed successfully. You don’t neccessarily need to do that for this specific task but it would have shown you exactly why this is not working for you.
    -Lines 50, 86, 140, 189, and 233: You should be checking if cond or if cond is True. (Optional)
    -Lines 200 and 244: You have assigned these as f-strings with nothing to be placed inside of them, recommended to remove the f in front of them. (Optional).
  • Lines 189 and 233: TypeError: 'bool' object is not callable the manage_guild attribute on permissions is not a method and should not be called like a method.
  • Line 86: You should be checking that the bot also has permission in the channel provided to embed_links. Alternatively adjust your sends in on_member_remove and on_member_join to account for that permission and optionally only send a text message instead.
  • Line 137: You should be checking that the bot has permission to embed links on this command you can do so simply with @commands.bot_has_permissions(embed_links=True).
  • Lines 185 and 229: You should be using logging instead of print. Prints will show up in the console but logging can store that information inside the bots logs. If this error message is critical for a bot owner to know they may not see it in the console and storing it in the logs is better long-term. (Optional)

redditpic

redditpic.py

  • Linea 73-98: You need to guard this command against sending NSFW content to non-NSFW channels.
  • This API you’re using appears to be down causing all commands to fail entirely and not provide a nice error message explaining such. I recommend adding except Exception: to catch any other errors alongside your ContetTypeError or replace it if this service comes back.
#6

I do not have time at this moment to fix InviteTracker and its problems, so that is currently disabled until I can find time to fix it. I also learned today that the API for RedditPic is dead, and it is now deprecated and removed from my repo. I may be bringing it back, but at this moment I have no intention of doing so.

Have a good day and thank you for the review :slight_smile:

#7

With the release of 3.5, we are returning to reviewing Cog Creator applications.

Since 3.5 will break every cog that was not specifically coded with it in mind, you need to ensure your repo is updated to be compatible with 3.5 prior to a review being possible. The relevant documentation you will need to update your repo is the following:
-discord.py 2.0 migration: https://discordpy.readthedocs.io/en/v2.2.3/migrating.html
-Red 3.5 breakages: https://docs.discord.red/en/stable/incompatible_changes/3.5.html#incompatible-changes-3-5

Once your repo is updated to support 3.5 on the default branch, please make a comment in this thread to confirm you are once again ready for review. We will only be reviewing repos which comment to confirm 3.5 support, but we will prioritize repos that have been in the queue for a longer period of time first.

If you wish to revoke your application, please comment accordingly so we can clean up the thread. If you need time to make changes, but are actively working on making updates, please also comment accordingly. Applications which do not see any activity within 1 month from today will be closed.

#8

Due to a lack of response, I will be closing this application. You may re-apply once your repo is updated to support the latest version of Red.

#9