[APPROVED] Karlo-cogs

Applications
#1

Discord Name: karlo

GitHub Repository (Must be V3): GitHub - karlsbjorn/karlo-cogs: Cogs for Red

Description:
Of note are two cogs related to World of Warcraft, one cog similar in function to the core Streams cog for sending a message when someone starts Discord streaming, and one game cog WikiArena where users guess which of two wikipedia articles has more views or words.

#2
#3

Thank you for your patience.

Commit hash: 9bd5e7eac7d7a25cedbfe761249e9a14daddaabd
Red v3.5.18
Consider only bullets prefixed with :memo: to be required.

General

Autoplay
N/A

DiscordStreams

  • :memo: The cog class is missing a docstring.
  • :memo: An error appears in console when the bot does not have embed_links perms in the alert channel:
    Traceback (most recent call last):
  File "datapath\cogs\CogManager\cogs\discordstreams\discordstreams.py", line 158, in update_guild_embeds
    await self.update_message_embeds(guild, member, message)
  File "datapath\cogs\CogManager\cogs\discordstreams\discordstreams.py", line 198, in update_message_embeds
    current_embed = message.embeds[0]
IndexError: list index out of range
    • This is because a message with both content and embeds will silently ignore the embed if the bot does not have that permission. Later, the embed you expect to be there will not exist.
  • If a role is passed to [p]discordstreamset mention, the alert uses the syntax of a user mention rather than a role mention.
  • The task name is incorrect in your .error method logging.
  • Your task does not check if the member that was streaming left the server.

NetDataAlerts
N/A

PylavChannelStatus
N/A

PylavRPC
N/A

SilentReplier
N/A

W3Champions

  • This cog is slash-only, which breaks convention.
    • :memo: This should be stated in the install_msg to make it clear what a user will need to do to use the cog (sync)
    • The single command in this cog should probably be red_force_enabled to remove the step of enabling the command if it is going to be slash only.

WarcraftLogsRetail

  • :memo: The cog fails to load on python versions below 3.10 because a vertical bar union type hint is used on line 531 of core.py. You should either change this typehint or restrict the python version in your info.json, as you have for other cogs.
    Traceback (most recent call last):
  File "\redbot\core\core_commands.py", line 189, in _load
    await bot.load_extension(spec)
  File "datapath\cogs\CogManager\cogs\warcraftlogsretail\__init__.py", line 3, in <module>
    from .core import WarcraftLogsRetail
  File "datapath\cogs\CogManager\cogs\warcraftlogsretail\core.py", line 32, in <module>
    class WarcraftLogsRetail(commands.Cog):
  File "datapath\cogs\CogManager\cogs\warcraftlogsretail\core.py", line 531, in WarcraftLogsRetail
    def humanize_dps(dps: int | float) -> str:
TypeError: unsupported operand type(s) for |: 'type' and 'type'
  • It might be better to restrict [p]wclset channel to moderators/guild owners.

WikiArena

  • This cog is slash-only, which breaks convention.
    • :memo: This should be stated in the install_msg to make it clear what a user will need to do to use the cog (sync)
    • The single command in this cog should probably be red_force_enabled to remove the step of enabling the command if it is going to be slash only.
  • Consider linking to the articles after the round/game is finished.
  • ButtonsView creates an aiohttp.ClientSession.
    • This session is used when it calls WikiArena.game_setup on line 343, since it passes self as the object to that method. This is bad practice, as self is supposed to be an instance of the class the method is a member of, and ButtonsView != WikiArena. It is possible that at some point, you would try to use an attribute that does not exist in ButtonsView, resulting in an error.
    • :memo: This session only gets closed in a separate cog_unload method that is never called, since ButtonsView is not a cog.
      • Be careful to check session.closed before using it in your view depending on how you fix this, since existing views will continue to run even after the cog is unloaded.
    • You probably should consider one of these two options:
      • Move the methods that manage the game (ie, make_wiki_embeds and the methods it calls) to all be within your view, since the view currently holds all of the game state. The command can then create a view instance when a game is started and call the methods using that object, and the view can call the methods on itself.
      • Pass the cog instance instead of config to the view, and use that object to get all of the needed attributes (config, session, the cog instance itself for its methods).
  • It might make sense to make the comparisons >= instead of >, to prevent an unlikely situation where two articles have the same views/words from resulting in an impossible choice.
  • Consider self.stoping the view in end_game, rather than using a game_status flag in the on_timeout method.

WoWTools

  • Some of the guild specific commands are missing guild_only checks.
  • The countdown dates have passed at this point
    • If another countdown for a future patch is going to be included, you may want to avoid defining the date(s) in multiple places.
  • The cn region is still included as an option, but I believe it is no longer valid.
  • :memo: Some places (ie, [p]price, [p]gmset view, the guild_log task, etc) do not properly check if the bot has embed_links perms.
#4

Hey Flame, thanks for the review!

I’ve made the required changes and many of the optional ones you’ve mentioned, except for WikiArena which I’ve decided to set as hidden since I believe it makes more sense to completely rewrite the cog once d.py supports components V2.

Let me know if there’s any other changes you’d like made!

#5

Thank you for making the requested changes.

WikiArena should be flagged to require a minimum python version of 3.10, as vertical bar union typehints were used in your updates. Since the cog is hidden, I am not going to worry about it for this review.

I am marking this as approved. Please allow some time for all of the associated bookkeeping.