Crab-cogs

orchidalloy · October 17, 2022, 1:39am

Discord Name: Orchid#1999

GitHub Repository (Must be V3): https://github.com/orchidalloy/crab-cogs

Description: Cool features that I developed for my own friend group (called crab).

Flame442 · May 4, 2023, 6:23pm

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.

orchidalloy · May 5, 2023, 11:51pm

My cogs support 3.5 and are ready for review

orchidalloy · May 10, 2023, 4:42pm

By the way my discord username is now holostrawberry#6743

For reference my user ID is 871733390251012147

TrustyJAID · June 26, 2023, 10:38pm

TrustyJAID · March 18, 2024, 7:23pm

Hi hollowstrawberry, thank you for your patience while I review your repo.

Commit hash at time of review - f37175c6498593054e4aab7ee4be2387ac24ab83

audioslash

info.json

The youtube-search-python repository appears to have been archived due to no maintainers. While this isn’t necessarily a problem it could mean that these features break at any time without a forseable fix. I also noticed browsing the repository that it has API tokens within the code itself. I am not sure how important these tokens are for the function of it but if they’re public like this it’s even more likely that they stop working since API tokens should typically be provided by the end user to access services like YouTube. Instead I recommend using some features supplied by Red. This is a bit more complicated but you only use this for autocomplete results but this should work.

import json
from redbot.core.cogs.audio.errors import YouTubeApiError


@play.autocomplete("search")
@playlist_add.autocomplete("track")
async def youtube_autocomplete(self, itx: discord.Interaction, current: str):
    cog = itx.client.get_cog("Audio")
    api = cog.api_interface.youtube_api
    SEARCH_ENDPOINT = "https://www.googleapis.com/youtube/v3/search"
    params = {
      "q": "current",
      "part": "snippet",
      "key": await api._get_api_key(),
      "maxResults": 20,
      "type": "video",
    }
    result = []
    async with api.session.request("GET", SEARCH_ENDPOINT, params=params) as r:
        if r.status == 400:
            if r.reason == "Bad Request":
                raise YouTubeApiError(
                    "Your YouTube Data API token is invalid.\n"
                    "Check the YouTube API key again and follow the instructions "
                    "at `{prefix}audioset youtubeapi`."
                )
            return result
        elif r.status == 404:
            return result
        elif r.status == 403:
            if r.reason in ["Forbidden", "quotaExceeded"]:
                raise YouTubeApiError(
                    "YouTube API error code: 403\nYour YouTube API key may have "
                    "reached the account's query limit for today. Please check "
                    "<https://developers.google.com/youtube/v3/getting-started#quota> "
                    "for more information."
                )
            return result
        results = await r.json(loads=json.loads)
    for res in results["items"]:
        result.append(app_commands.Choice(name=res["snippet"]["title"]), value=res["id"]["videoId"])
    return result

Getting the information this way lets you utilize the bot onwers API token and removes the archived dependancy. You can look more at the YouTube API documentation here to see if you can get other information you might want to include such as the duration which I didn’t see from this example. It’s also important because YouTube tracks may be disabled on the bot entirely so it would be good to implement a similar approach for soundcloud instead, for example.

I would suggest that you mention this cog consumes 9/100 application commands a bot is allowed to have in the install message. One of the biggest limitations of slash commands is this 100 command limit bots have. I would encourage you to make all of the commands fit underneath one top level audio command rather than keep the simple commands like /play and /skip make them fall under /audio skip and /audio play instead. This reduces it from 9/100 to 1/100 since each command can have 25 subcommands or groups and each subcommand can have a further 25 subcommands. I understand it’s not as ergenomic as just using /play but for a multi-purpose bot like Red it’s an important consideration since people can install any number of cogs and commands on their bot and removing 1/10th of the slash command limit for a few small commands feels a bit harsh. (Optional)

audioslash.py

Lines 298 and 327: You should not use bare except, at least make it except Exception if you are unsure what you want to catch and will catch everything. I would encourage you to investigate what actual errors you may receive as well so you might handle it differently. Simply logging the behavior is generally also not enough as this will lead to discord’s interaction failed response and is confusing to end users. It would be better to explain that an error happened to them explicitly so it does not appear that the command just doesn’t work when used improperly to cause the error.

autoreact

autoreact.py

Line 56: You should not allow user defined regex to be unregulated like this. A server admin could define a regex pattern that is catastrophically backtracking. This would mean any time someone sends a message the bot could indefinitely hang for everyone.

There are ways to avoid this issue namely the re2 module from google is designed to prevent this but that is complicated to install and has some missing useful regex features. Another approach might be to spin up the regex searching in a process pool and let a whole new python interpreter run the regex and if it takes too long don’t do anything on that message. An example of bad regex would be ^(?:a+)+$/ and someone sending a message with about 100 a’s. This pattern will compile correctly according to your original checks but would cause the bot to hang attempting to search the message. For an example of the process pool in action you can see my ReTrigger cog.

Lines 111-112: You check the length of the pattern before stripping code blocks if they’re present. You could check the length after stripping though you should also probably remove newline characters when code blocks are removed as well so that the pattern doesn’t start with \n(?i)my pattern failing to build correct regex. This length seems arbitrary too, I don’t see any reason to limit the pattern to only 400 characters when users can send messages up to 4000 characters with Nitro. Embed descriptions were also updated to handle up to 4096 characters when users were given up to 4000. The length of the pattern has minimal effect on performance. (Optional)
Line 147: You have no checks for embed links and only send embeds. If the command is run in a channel where the bot is missing embed_links permission this will fail.
Lines 160-163: If SimpleMenu is passed a list of length 1 it will only provide the close button instead of the navigation buttons. It might be better to use it for consistency incase the user wants to remove the message and doesn’t otherwise have manage_messages permission. (Optional)
Line 142: You use ctx.react_quietly without passing a fallback message. If the bot does not have add_reactions permission then the user might be confused since the bot won’t do anything. The only permission Red guarantees the bot has on commands is send_messages, everything else you must check yourself. Furthermore you pass the reaction when you can instead use ctx.tick() which will function the same as react_quietly and even allows you to pass the message if the bot does not have permission to add reactions.
Lines 69, 164, 170: You have leftover whitespace. This isn’t an issue for python but more of a styling recommendation that you don’t have trailing whitespace in your code. Most Python projects request that you remove extraneous whitespace when contributing and it’s easy to setup with your code editor of choice. (Optional)

draw

draw.py

Line 49: If a user tries to use this command in a channel where the bot does not have attach_files permission the text command version will fail.
Line 24-29: You might consider saving the image in memory with BytesIO instead of saving to disk. This can save unnecessary cleanup after the fact. (Optional)
Lines 59-70 and 87-98: All of this code is technically blocking meaning if another command is run while this is processing the bot won’t be able to respond to anything else. Image manipulation is typically slow to process which is what makes this problematic. The recommended way to solve this is to run the code in an executor like:

import asyncio
loop = asyncio.get_running_loop()
data = await loop.run_in_executor(my_long_task, timeout=10)

easytranslate

info.json

Your end user data statement should mention it stores users preferred language to translate to.

easytranslate.py

Lines 106 and 168: You should not use bare except, at least make it except Exception if you are unsure what you want to catch and will catch everything. I would encourage you to investigate what actual errors you may receive as well so you might handle it differently.
Line 56: You do infact store end user data here in the form of preferred language. This data should be cleared here or otherwise this method should not exist on the cog since the cog is not handling end user data correctly. This is as simple as await self.config.user_from_id(user_id).clear().

emojisteal

emojisteal.py

Line 156: You should not use bare except, at least make it except Exception if you are unsure what you want to catch and will catch everything. I would encourage you to investigate what actual errors you may receive as well so you might handle it differently. In this case since you are checking add reactions permission before the command is run there’s only a few instances this can error but it can still error and should be handled.
Line 138: This is a bit of an odd way to go about stripping characters that discord doesn’t allow for emoji names. I would recommend simply doing names = [re.sub(r"\W+", "", name) for name in names]. Note the \W+ will capture the opposite of your pattern or any non-word character instead of looking for only word characters and automatically substitute nothing in its place. (Optional)
Line 77: You pass a or '' instead of a boolean into the StolenEmoji constructor which is inconsistent with your typehint. In practice this should be fine since the empty string will be treated as false and the a will be treated as true but it is inconsistent. I would recommend building a constructor that explicitly translates the string into an actual boolean. You might also consider making this staticmethod a classmethod for StolenEmoji since it handles construction, something like

@dataclass(init=True, order=True, frozen=True)
class StolenEmoji:
    animated: bool
    name: str
    id: int

    @property
    def url(self):
        return f"https://cdn.discordapp.com/emojis/{self.id}.{'gif' if self.animated else 'png'}"

    def __hash__(self):
        return hash(self.id)

    def __eq__(self, other):
        return isinstance(other, StolenEmoji) and self.id == other.id

    @classmethod
    def from_str(cls, emoji_str: str) -> StolenEmoji:
        # Loosely copied from https://github.com/Rapptz/discord.py/blob/master/discord/partial_emoji.py#L116-L149
        match = re.match(r"<(?P<animated>a?):(?P<name>\w+):(?P<id>\d{13,20})>", emoji_str)
        groups = match.groupdict()
        animated = bool(groups['animated'])
        emoji_id = int(groups['id'])
        name = groups['name']
        return cls(name=name, animated=animated, id=emoji_id)

This will then parse any emoji string you pass it into the StolenEmoji object you can use for whatever and correctly convert animated into a boolean instead of leaving it as strings which loosely translate to the boolean. I should also note that this entire dataclass can be replaced with discord.py’s PartialEmoji object which has a from_str method already built to parse emoji strings into a Partial Emoji object providing everything you have here and more. I would recommend you look at using that instead throughout this cog since it even provides proper access to discord CDN assets allowing saving and copying. (Optional)

Lines 78, 107, 125, 137, 168, 170, and 203: Again, you have leftover whitespace. (Optional)

gamealert

gamealert.py

Line 42: You would be better to only iterate over saved guilds instead of all of the bots guilds. Right now as a bot grows this wastes cycles iterating over many guilds that it would otherwise skip. Using bot.get_guild(guild_id) to get the guild object is an O(1) operation while this is O(n). This is because it’s iterating through every guild (n) the bot is in to see if any are setup when it already knows which ones are setup and can just get the guild object freely. (Optional)
Lines 94 and 106: You use react_quietly without a message response so if the bot is missing add_reactions permission this will error and look like the command did nothing at all.Furthermore you pass the reaction when you can instead use ctx.tick() which will function the same as react_quietly and even allows you to pass the message if the bot does not have permission to add reactions.

gelbooru

gelbooru.py

Lines 145 and 168: You might consider using the params argument instead of constructing the url as a string. This simplifies access to API’s where the url parameters don’t need much for special formatting and aiohttp will format it correctly for you. For example:

url = "https://gelbooru.com/index.php"
params = {
    "page": "dapi",
    "s": "tag",
    "q": "index",
    "json": "1",
    "sort": "desc",
    "order_by": "index_count",
    "name_pattern": query,
    "api_key": api_key,
    "user_id": user_id,
}
async with aiohttp.ClientSession(headers=HEADERS) as session:
    async with session.get(url, params=params) as resp:
        data = await resp.json()

As you can see it’s a lot easier to read and change parameters this way. I should also recommend that you make a cog-wide session rather than constructing a new session for every request. This is just good practice. (Optional)

Line 67 and 131: You should not use bare except, at least make it except Exception if you are unsure what you want to catch and will catch everything. I would encourage you to investigate what actual errors you may receive as well so you might handle it differently. In this case you might want to parse the individual API error responses in the API methods such as 401 unauthorized or 429 too many requests. Although in practice I only ever proceed if it’s 200 and anything else I log and return a generic error message to the user. It’s up to you how you handle the specific error’s I only care about the bare except here.
Line 18: You have an f-string defined but no placeholders in the string itself. (Optional)

imagelog

imagelog.py

With this cog it should be mentioned somewhere in the setup commands that re-posted images from the bot like this cannot be used for reporting users to discord. If an image has content that breaks discord TOS it needs to be reported alongside deleting the message or reported prior to the message being deleted, otherwise once deleted the message is gone for good with no way for discord to prove who sent it originally which ultimately makes re-posting it for moderation purposes a moot point other than making moderators spend more time looking at something they probably don’t want to look at. This makes handling this sort of thing tricky and while you mention it as an issue to the bot owner in the install messages this information needs to be clearly displayed to everyone who might consider using this function.
Line 43: There’s no reason to limit this since embed descriptions can have up to 4096 characters now.
Line 66: You should not use bare except, at least make it except Exception if you are unsure what you want to catch and will catch everything. I would encourage you to investigate what actual errors you may receive as well so you might handle it differently.

imagescanner

constants.py

Lines 10 and 32: You have f-string’s defined with no placeholders. (Optional)

imagescanner.py

Lines 143, 221, and 311: You should not use bare except, at least make it except Exception if you are unsure what you want to catch and will catch everything. I would encourage you to investigate what actual errors you may receive as well so you might handle it differently.
-Lines 162 and 166: You can error here if the user does not allow DM’s in the server or has the bot blocked.
Lines 167-169: Why are you manually handling these view timeouts? You can modify the view’s on_timeout method to automatically edit and remove the buttons from it. (Optional)

utils.py

Lines 22, 61, and 65: You should not use bare except, at least make it except Exception if you are unsure what you want to catch and will catch everything. I would encourage you to investigate what actual errors you may receive as well so you might handle it differently.

logs

logs.py

Line 56: Two hours seems like a long time for something that is changing behind the command itself and is only showing a snapshot in time especially since the view automatically refreshes the timeout every time it is interacted with. (Optional)

novelai

novelai.py

Line 19: You should avoid using star imports, try to be explicit about what you are importing and what you actually need. Typically star imports are used in libraries which then re-export the starred imports in a defined way. In this instance linters and type checkers see the star import and don’t know if something is now undefined and have to assume it’s from a star import which makes reading and understanding the code more challenging. (Optional)
Line 73: You need to be clearing the users configured settings here if you want to include this method in the cog. Alternatively you can remove this method if you don’t want to clear the user’s configured settings when they ask to have their data removed. If you choose the latter they will at least be notified that this information was not cleared when the delete data command is run and can theoretically adjust personal information themselves through the commands that change it. This is because, while AI input prompts generally won’t contain personal information, it is still information provided by a user who may wish all information they have provided to the bot to be deleted including historical AI prompts. If you wish to keep it for moderation purposes that is why the requester argument is provided so that the data can be treated more carefully and only truly deleted when the owner has asked to have it deleted or some other condition based on the person requesting data deleted. The point here is that Red isn’t lying to the user when they request their data deleted and having this method in the cog doing nothing is lying when you have potentially saved something from them.
Lines 95, 206, 274, 382, 388, and 506: You should not use bare except, at least make it except Exception if you are unsure what you want to catch and will catch everything. I would encourage you to investigate what actual errors you may receive as well so you might handle it differently.
Line 192: You might want to defer earlier in this method. The prepare_novelai_request method looks like it could easily take longer than 3 seconds to return which would cause the command to fail when used as a slash command. (Optional)
Line 240: I personally find it odd that you chose to use calendar and generate the timestamp this way. There’s a much simpler method via discord.utils.format_dt(eta, "R") to generate the timestamp string for discord. Alternatively you already have the datetime object so it can also be f"<t:{int(eta.timestamp())}:R> both of which don’t require importing calendar which is only used for this. (Optional)
Line 373: You might consider utilizing the view’s on_timeout method to handle this instead of creating a task that wait’s and removes the buttons instead. (Optional)

randomness

randomness.py

Line 93: You use ctx.react_quietly without a backup message response and without checking permission to add reactions potentially causing this to fail and provide no response when run. These are owner only commands but you can’t guarantee that a bot owner has chosen to run the command in a channel the bot has add reaction permission in. Furthermore you pass the reaction (and even define it as a module varaible!) when you can instead use ctx.tick() which will function the same as react_quietly and even allows you to pass the message if the bot does not have permission to add reactions.

TrustyJAID · March 18, 2024, 7:23pm

simulator

This is a bit of a tricky cog to handle. I see you have made it opt-out but in this case it should really be an opt-in cog. I know this decreases usability but even though it’s easy to opt-out it can be difficult for regular users to even know this data is being used and therefore it should be strictly opt-in. For example the cog may be setup to track and process the data from a user but not setup to be interacted with or long forgotten. A new user might join and start talking entirely unaware that this data is being processed and potentially even leave not knowing this data was collected and now impossible for them to remove the data. You should not be doing things that a user is unaware of especially if that information is collected and later construed into new forms. I know this is meant for fun but these are all things that need to be considered by bot owners when setting up a cog like this. It’s not enough to just require a role to opt-in because roles are a bit more dynamic and can inadvertantly add users who still fall within this criteria since role assignment is handled differently. One could even set the @everyone role as the opt-in role forcing everyone to participate not to mention auto-roles from other cogs/bots and other means in which the user is opted-in without their consent.

simulator.py

Line 18: You should avoid using star imports, try to be explicit about what you are importing and what you actually need. In this case the preferred method of importing everything you aren’t sure you need from typing is to just import typing then use things like typing.Optional when you need them. This way linters and type checkers aren’t assuming that something undefined is imported from a star import. (Optional)
Line 459: You should not make one-line statements like this. It decreases readability especially since this also raises KeyError making it more challenging to find issues in the code. (Optional)
Lines 328, 339, 347, 355, and 363: You use ctx.react_quietly without a backup message response and without checking permission to add reactions potentially causing this to fail and provide no response when run. These are owner only commands but you can’t guarantee that a bot owner has chosen to run the command in a channel the bot has add reaction permission in. Furthermore you pass the reaction (and even define it as a module varaible!) when you can instead use ctx.tick() which will function the same as react_quietly and even allows you to pass the message if the bot does not have permission to add reactions.
Line 471: You don’t check if the bot has permission to manage_webhooks in the channel.

tts

tts.py

Lines 35 and 66: You should not use bare except, at least make it except Exception if you are unsure what you want to catch and will catch everything. I would encourage you to investigate what actual errors you may receive as well so you might handle it differently.
Lines 54-59: The two libraries you are using here, googletrans-py and gtts are not async compatible. So this block of code is technically blocking, meaning the bot will not respond to anyone else while it’s waiting to get a response from the associated API’s. The recommended way to solve this is to run the code in an executor or find a library that supports asyncio. The googletrans-py library at least uses httpx which does support asyncio but the library has zero mention of asyncio support and does not appear to allow it to be used in such a way. The gtts library instead uses requests which has zero asyncio support.

voicelog

voicelog.py

Line 42: You have leftover whitespace. This isn’t an issue for python but more of a styling recommendation that you don’t have trailing whitespace in your code. Most Python projects request that you remove extraneous whitespace when contributing and it’s easy to setup with your code editor of choice. (Optional)
Lines 64 and 71: This time you at least chain the add_reactions permission check from the command group. However, this also means that the command will not be visible to users if run inside a channel where the bot is missing the permission which can be confusing since the command does not strictly require add_reactions permission for anything other than confirming the command was run. I still recommend that you add a confirmation message to react_quietly and remove the permission check so that the command is more usable and the bot responds appropriately when the command is run. Furthermore you pass the reaction when you can instead use ctx.tick() which will function the same as react_quietly and even allows you to pass the message if the bot does not have permission to add reactions. (Optional)

orchidalloy · December 17, 2024, 4:43am

Hello, thank you for your review and all sorts of useful feedback. It’s been a while, initially the review was quite discouraging, but since then I’ve picked up the pace, made some changes including those you mentioned, and I have also added a few more cogs, which I hope are up to standards. I’ll go over each cog and describe the changes I’ve made.

common

I have specified the errors I want to catch in all of my uses of except throughout the codebase. Sometimes I find it preferable to catch all errors, so I switched to except Exception like you said; usually to have something to show to the user instead of Red’s builtin “an error has occured”, and also for loops I don’t want to be entirely interrupted by an exception.
I now use ctx.tick() whenever possible instead of my previous approach.
Use of red_delete_data_for_user has been improved, and removed if it’s unused.
I formatted the files a little better whenever I thought it would improve readability.