[APPROVED] Mr42-cogs

[Mr.42]

Discord Name: Mr. 42#1337

GitHub Repository (Must be V3): GitHub - Mister-42/mr42-cogs: Cogs for Red - Discord Bot

Description: Learning Python. Just one cog for now (posting newly released YouTube videos from selected channels to guild channel).

[Flame442]

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.

[Mr.42]

I have made my mods compatible with 3.5 quite a while ago. No additional changes are required at this point, as they run well.

[Mr.42]

Discord name updated to mr42.yt
id 823288853745238067

[Flame442]

Thank you for your patience.

Commit hash: d0b4b31f28cbcadb8628d82fffddc118f42dceee
Red v3.5.2
Consider only bullets prefixed with to be required.

Avatar

• The cog install_msg states to use [p]avatar help for a description of the cog, however [p]avatar does not have a [p]help subcommand nor does it have special handling for passing help, so this is equivalent to just [p]avatar unless a user named [p]help exists in the server. You might want to change that message to [p]help avatar, or [p]help Avatar.
• [p]avatar raises an error if used in a channel where the bot does not have attach_files permissions:

    File "datapath\cogs\CogManager\cogs\avatar\avatar.py", line 45, in avatar
      await ctx.send(message, file=discord.File(pfp, filename=filename))
  discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
  

• You might want to add sub/commands to fetch the different avatars of a user, their global avatar vs their guild avatar override.
  • Using the discord.Member converter in DMs causes a random member object for a guild that user is in to be used. When the command is used on a target member in DMs, you may want to explicitly fetch their base avatar, rather than their display avatar, to avoid using an avatar from a particular guild.
• You should unindent the last send so it is outside of the typing block. That way, you make sure that another typing event is not sent after the message is sent, so the message properly stops the bot from typing.
• Rather than <@{ctx.author.id}>, you can use {ctx.author.mention}.

Youtube

• The cog install_msg states to use [p]youtube help for a description of the cog, however [p]youtube does not have a [p]help subcommand, so this is equivalent to just [p]youtube. You might want to change that message to [p]help youtube, or [p]help YouTube.
• You should add bounds checking to your config commands. For example, [p]youtube maxpages -100 is a valid command currently.
• [p]yt list and [p]youtube listall raise an error if used in a channel where the bot does not have attach_files permissions and the maxpages setting is exceeded:

    File "datapath\cogs\CogManager\cogs\youtube\youtube.py", line 427, in listall
      await self.list(ctx)
    File "datapath\cogs\CogManager\cogs\youtube\youtube.py", line 198, in list
      return await ctx.send(file=page)
  discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
  

• [p]yt info and [p]yt infoall raise an error if the length of the channels + custom messages exceeds 6000 characters:

  Traceback (most recent call last):
    File "datapath\cogs\CogManager\cogs\youtube\youtube.py", line 403, in infoall
      await self.info(ctx, channelYouTube)
    File "datapath\cogs\CogManager\cogs\youtube\youtube.py", line 323, in info
      return await ctx.send(file=icon, embed=embed)
  discord.errors.HTTPException: 400 Bad Request (error code: 50035): Invalid Form Body
  In embeds: Embed size exceeds maximum size of 6000
  

  • A similar error happens in a channel where the bot does not have embed_links permissions, but for the 2000 character limit.
• [p]yt custom with a channel passed but no message raises an error due to the default value of False not being a string, with no special handling:

  Traceback (most recent call last):
    File "datapath\cogs\CogManager\cogs\youtube\youtube.py", line 214, in custom
      msg = message.replace("\\n", "\n")
  AttributeError: 'bool' object has no attribute 'replace'
  

• [p]yt delete and [p]yt migrate silently raise an error when used in a channel where the bot does not have add_reactions permissions:

  Traceback (most recent call last):
    File "c:\users\flame\venv\live\lib\site-packages\redbot\core\utils\menus.py", line 328, in task
      await message.add_reaction(emoji)
    File "c:\users\flame\venv\live\lib\site-packages\discord\message.py", line 1098, in add_reaction
      await self._state.http.add_reaction(self.channel.id, self.id, emoji)
    File "c:\users\flame\venv\live\lib\site-packages\discord\http.py", line 738, in request
      raise Forbidden(response, data)
  discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
  

• I’m not quite sure why [p]yt migrate has a call to bot.unload_extension for Tube in it. That cog should not be possible to load at the same time as this cog, due to a command name conflict.
• The background task attempts to DM guild owners if a youtube channel gets deleted, however this does not have a check that there is no HTTPException due to the bot being blocked or DMs from server members being disabled. This send should properly handle this error to prevent the task from being killed due to a failed DM.

YoutubeDedup

• The cog install_msg states to use [p]youtubededup help for a description of the cog, however [p]youtubededup does not have a [p]help subcommand, so this is equivalent to just [p]youtubededup. You might want to change that message to [p]help youtubededup, or [p]help YouTubeDeDup.
• The cog will fail to process_message if no youtube video links were sent in the last history days. The only logic for whether a channel is “enabled” is whether or not a message exists in config.channel().messages. If no youtube videos were sent recently, this will not be filled with anything, and new messages will not be processed.
• The history fetching logic for [p]ytdd watch does not explicitly set the limit flag, so only the oldest 100 compatible messages are searched.
• You might want to add special logic handling the case of wrapping a youtube URL in <link> in order to suppress the embed.

[Mr.42]

Sorry for the massive delay. I had life going on

• Avatar
  all points changed/fixed

• Youtube
  Tube and my cog can be loaded at the same time, as they don’t share a namespace
  all other points changed/fixed

• YoutubeDedup
  A change in history currenly does not change the saved data indeed. It’s on the todo list
  I don’t seem to get an error in case of an empty list {} in process_message
  I don’t quite get the last point

[Flame442]

Commit hash: 8f296169006b413ea8db93bdcafe81cebc2ae717
Red v3.5.4
Consider only bullets prefixed with to be required.

Youtube

• The bounds checking you added to config commands only applies to the user output. The negative/zero value is still stored in config, since limit is stored, rather than maxPages.
• Bare excepts can catch exceptions like KeyboardInterrupt that are necessary for normal control flow. More specific error types should generally be used where they are known. In cases where you do not know what exception might be raised, you should still use except Exception to avoid catching those system exceptions.

YoutubeDedup

• The bug with process_message happens because the config value messages is doing double duty as both the config value to remember what channels are being watched, as well as being the config value to remember what links were previously sent. In cases where a channel is set to be “watched”, but no links were present, this config value will remain empty, and the cog will think that the channel was not set to be watched, so new links will not be recorded. To reproduce the bug:
  1. Create a new text channel.
  2. Run [p]youtubededup watch <channel> on that new channel.
  3. In the new channel, send the same youtube link two times. Notice that it does not delete the second link as expected.
  4. Run [p]youtubededup watch <channel> on that channel a second time. Notice that it now deletes the second link.
  5. Send the same youtube link a third time. Notice that it does delete the link.
• When a link is sent in the discord client in the form <link> (ie <https://google.com>), it automatically prevents the link from embedding, without needing to manually hit the x on the embed to remove it. It might make sense to make your link detection properly handle links that have their embeds suppressed in this way.

[Mr.42]

The required points are changed/fixed
Suppressed links required a regex update. I have tested the solution with various entries and got good result, but I’m never 100% sure about regex.
The except will be looked into

[Flame442]

In the change you made for ytdedup, all_channels is not defined in your defaults, nor is it updated in [p]youtubededup watch, so the cog still does not behave as expected.

[Mr.42]

all_channels is a function. I have testing this after removing settings.json and on a new channel and it works as expected. Is there a situation I missed?

[Flame442]

That’s what I get for reading the code without testing it, and forgetting a config builtin…

You’re all set, I’m marking this as approved now. Cog Creator perks will be rolled out over the next 24 hours.

[Mr.42]

Thank you very much for your time, efforts and directions.