[APPROVED] Toxic-Cogs (2: Reapplication)

#1

Discord Name:
Neuro Assassin#4227 <@473541068378341376>
GitHub Repository (Must be V3):


Description:
Updates from latest application:
1. simon and twenty now allow multiple games to be played at once.
2. namechecker has been removed due to being a poor excuse of a cog.
3. webstatus has been removed due to outage.report changing class names that they have on their site
4. commandchart now correctly grabs all sub-commands of command messages.
5. color (new): Provide either one of the following of a color: name, hexadecimal value, rgb value or hsl value; and it will respond with the hexadecimal value, rgb value and hsl value in an embed with its color being set to that color.
6. minesweeper (new): allows you to play Minesweeper inside of Discord (non-spoiler, due to that not being available on the API side of Discord). EDIT: it now has a command that allows you to play it with spoilers, though it will not be able to keep track of bombs.
7. sql (new): allows you to store data inside of a table in either the bot’s memory or in your server’s file. You can also control who can edit the table, who can select data from the table. Server owners can also run raw sql commands, though that is discouraged.

Hopefully this fixed many of the needed changes from my last application.

#2

Quick Update: I discovered a small issue with a path in my sql cog, just fixed it with a commit (hash: 308462d2aeba83b2d37af306295d838cab83570c). Also cleaned up some str()s.

#3

Another quick update: I added a feature in my sql cog where now you can specify the edit and select roles as 0, which will allow for anyone to edit/select that table.

assigned Redjumpman #4
#5

@Neuro_Assassin I’ll be reviewing your application in the coming days. Just wanted to let you know I’ve assigned myself to this app. Expect to see a preliminary review soon.

unassigned Redjumpman #6
assigned Flame442 #7
#8

Hi @Neuro_Assassin, I’ve been assigned to take over review of your application.

Commit hash: a916bbfdef96045f6af4be988639d929ceae5f34
Red v3.1.1
Consider anything prefaced with “You might want” to be optional

Color
-You do not have a class doc string, so [p]help Color does not have a cog description.
-You might want to limit the number of decimals in your floats to make the numbers more readable.
-[p]color msgshort should either be disabled by default or there should be a warning in the install message that it starts off enabled.
-What [p]color msgshort does and how to trigger it needs to be explained better in the help text.
-If [p]color msgshort is enabled, using [p]color hex <hex value prefaced with #> will cause both the invoked command and [p]color msgshort to respond at the same time.
-If the bot does not have permission to send embeds, all of the commands will only send a square of the color. You need to handle that in a better way.
-The __author__ var does not have your current discrim (4227 vs 4779).
-The cog uses pillow, however it is not in your requirements in your info.json.
-As far as I can tell, rgb2hex is not a valid import from colour.

Commandchart
-You do not have a class doc string, so [p]help CommandChart does not have a cog description.
-The __author__ var does not have your current discrim (4227 vs 4779).
-You might want to use typing.Optional to allow users to see a certain number of messages w/o needing to specify the channel.
-The command errors if the bot does not have permission to send embeds:

"<datapath>\cogs\CogManager\cogs\commandchart\commandchart.py", line 105, in commandchart
    em = await ctx.send(embed=e)
discord.errors.Forbidden: FORBIDDEN (status code: 403): Missing Permissions

Cooldown
-You do not have a class doc string, so [p]help Cooldown does not have a cog description.
-If a cooldown is removed from a cog that had a cooldown not set from this cog, it is not kept on a bot restart.
-You should explain that adding a cooldown will overwrite the current cooldown (if one exists).
-A negative value can be passed to the rate value of [p]cooldown add.
-You might want to handle a command with a value being passed to [p]cooldown add. Right now, running [p]cooldown add 1 1m global play foo locks play to 1 per 1 min, ignoring foo.
-You have an arg parser in converters.py, but it does not look like it is ever used.
-The group command does not have any checks, so the command shows up in [p]help for all users.

Deleter
-You do not have a class doc string, so [p]help Deleter does not have a cog description.
-The help text for [p]deleter channel is a bit confusing and does not have good grammar. I don’t understand what this command does just by looking at the help text. The command sets up a channel to have auto-deleting messages, however the text only talks about “a message”.
-The help text for [p]deleter remove has bad grammar, specifically the phrase “Helpful for when announcements that you want to stay in the channel”.
-You might want to specify that the wait in [p]deleter channel is in seconds.
-You might want to specify that the messages in [p]deleter remove should be a message id.
-A good percentage of messages seem to get through the deletion. It could be a race condition in the on_message, or something with rate limits.
-You should consider using async with when you are managing the self.conf.channel(channel).messages dict.
-Why does [p]deleter wipe require admin? Someone could already disable the deleter with the base perms, so why does this specific command have elevated perms?
-The background task errors out and stops working if a channel is deleted that has a message set to be deleted:

"<datapath>\cogs\CogManager\cogs\deleter\deleter.py", line 33, in background_task
    m = await c.fetch_message(int(message))
AttributeError: 'NoneType' object has no attribute 'fetch_message'

Editor
-You do not have a class doc string, so [p]help Editor does not have a cog description.
-Passing a voice channel ID causes the command to error:

"<datapath>\cogs\CogManager\cogs\editor\editor.py", line 41, in editmessage
    editmessage = await editchannel.fetch_message(editid)
AttributeError: 'VoiceChannel' object has no attribute 'fetch_message'

-Passing a category channel ID causes the command to error:

"<datapath>\cogs\CogManager\cogs\editor\editor.py", line 41, in editmessage
    editmessage = await editchannel.fetch_message(editid)
AttributeError: 'CategoryChannel' object has no attribute 'fetch_message'

-Attempting to edit a message that is not by the bot AND specifying content (instead of a message to copy from) causes the command to error:

"<datapath>\cogs\CogManager\cogs\editor\editor.py", line 65, in editmessage
    await editmessage.edit(content=content, embed=None)
discord.errors.Forbidden: FORBIDDEN (status code: 403): Cannot edit a message authored by another user

-Attempting to edit a message in a channel the bot cannot access causes the command to error:

"<datapath>\cogs\CogManager\cogs\editor\editor.py", line 41, in editmessage
    editmessage = await editchannel.fetch_message(editid)
discord.errors.Forbidden: FORBIDDEN (status code: 403): Missing Access

Evolution
-You should warn the user that the cog requires the bank to be global sometime before the cog load.
-[p]evolution backyard 1 throws an error because you defined the paramater name as menu, overriding the menu var provided by red:

"<datapath>\cogs\CogManager\cogs\evolution\evolution.py", line 260, in backyard
    await menu(ctx, embed_list, DEFAULT_CONTROLS)
TypeError: 'bool' object is not callable

-[p]evolution backyard errors if the bot does not have permission to send embeds:

"<datapath>\cogs\CogManager\cogs\evolution\evolution.py", line 272, in backyard
    await ctx.send(embed=embed)
discord.errors.Forbidden: FORBIDDEN (status code: 403): Missing Permissions

-[p]evolution shop errors if the bot does not have permission to send embeds:

"<datapath>\cogs\CogManager\cogs\evolution\evolution.py", line 240, in shop
    await menu(ctx, embed_list, controls)
discord.errors.Forbidden: FORBIDDEN (status code: 403): Missing Permissions

-L318, you have a hardcoded animal name of “chicken”.

ListPermissions
-You do not have a class doc string, so [p]help ListPermissions does not have a cog description.
-You might want to allow role IDs to be used instead of just names.
-There are better ways to handle the places you sent two messages “in case the x name is really long”, such as pagify.

Maintenance
-You do not have a class doc string, so [p]help Maintenance does not have a cog description.
-You might want [p]maintenance deleteafter to require a value to be passed, setting it to 0 when no value is passed is a little confusing.
-[p]maintenance settings says that messages will be deleted after 0 seconds instead of saying that they will not be deleted.
-Negative values can be passed to [p]maintenance deleteafter
-An incredibly long message value in [p]maintenance message causes [p]maintenance settings to error:

"<datapath>\cogs\CogManager\cogs\maintenance\maintenance.py", line 126, in settings
    return await ctx.send(sending)
discord.errors.HTTPException: BAD REQUEST (status code: 400): Invalid Form Body
In content: Must be 2000 or fewer in length.

-If any users are whitelisted, [p]maintenance settings errors:

"<datapath>\cogs\CogManager\cogs\maintenance\maintenance.py", line 134, in settings
    user_profile = await self.bot.get_user_info(user)
AttributeError: 'Red' object has no attribute 'get_user_info'

-You might want to make it more clear that [p]maintenance whitelist only affects the current maintenance.
-[p]maintenance whitelist allows you to “add” users to the whitelist even though users should not be able to be added to the whitelist unless the bot is in maintenance since it only affects the current maintenance.
-You might want the error messages of [p]maintenance off and [p]maintenance on to be the same. Right now, only [p]maintenance on tells the user about [p]maintenance off.

Minesweeper
-You do not have a class doc string, so [p]help Minesweeper does not have a cog description.
-The __author__ var does not have your current discrim (4227 vs 4779).
-The help text of the two commands have bad grammar, specifically the phrase “with allowing you to”.
-The full help text of spoilerms is longer than 256 characters, so it is cut off. (Note: v3.1.2 removes the help text limit, but you still should probably do something about the size of this help text)
-You might want to say how many bombs are actually in a game, especially when it is randomly picked so user will know how many there are to know when they’ve “won”.
-You might want to silently ignore bad inputs in minesweeper so that the user can still chat while playing.
-An error is printed for every invalid space-separated guess in minesweeper, which can cause spam or hit rate limits.
-The board is edited for every space-separated guess in minesweeper, which can cause spam or hit rate limits.
-You need to put your warnings under the install_msg of the info.json.

Simon
-The __author__ var does not have your current discrim (4227 vs 4779).
-You have a group command with only 1 subcommand in a cog that hasn’t been updated for 2 months. I doubt you are going to add more subcommands, so why a group command?
-There are a lot of reasons why I don’t think this game is able to work in discord:
—The user has a text box to type the sequence as it appears.
—The rate limits make some of the “flashes” last for over 5 seconds.
—If the same number “flashes” twice in a row, the blank spot in between “flashes” can be incredibly short, making it hard to tell that it “flashed” again at all.
-In a normal game of simon, the sequence “builds” on top of the beginning sequence. In your game, each sequence seems completely random.
-Bad grammar in L53 (“due to no response for”).
-You might want to print the score if someone reacts with the X emoji.
-You only wait for reactions when waiting for the green check, X only works in between rounds.

SQL
-You do not have a class doc string, so [p]help Sql does not have a cog description.
-The __author__ var does not have your current discrim (4227 vs 4779).
-The group command [p]sql settings does not have any checks, so the command shows up in [p]help for all users.
-L1212, you do not have a catch for asyncio.TimeoutError
I didn’t review this cog in full for a few reasons:
1] I do not know SQL and do not feel confident reviewing it without understanding the basics of what it is doing and what it can do.
2] I’m not even sure what the use case is supposed to be, can you please explain what someone (or you) can use this cog for?

SW
-You do not have a class doc string, so [p]help SW does not have a cog description.
-The all commands do not seem to be sorted in any particular way, making them a bit more confusing.
-You might want to replace the “Hold on” messages with ctx.typing().
-You might want to add searching by name instead of just the id system.
-You redefine the built in next a couple of times.
-The commands all error if the bot does not have permission to send embeds:

"<datapath>\cogs\CogManager\cogs\sw\sw.py", line 187, in film
    await menu(ctx, embeds, DEFAULT_CONTROLS)
discord.errors.Forbidden: FORBIDDEN (status code: 403): Missing Permissions

Switcher
-You do not have a class doc string, so [p]help Switcher does not have a cog description.
-You might want to restrict this cog to DMs with @commands.dm_only() and add a warning in the install message that the cog is DM only.
-You might not want the [p]switcher list code block to have python syntax highlighting.

Targeter
-You do not have a class doc string, so [p]help Targeter does not have a cog description.
-You might want to have alternate outputs, such as different file formates or an IDs only option.
-You might want to allow certain params to be used without values. For example, --not-nick should be able to used with no params to see all members w/o a nick. Right now you have to pass a fake “nick” for it to test against for that to work.
-The date parser seems to be a bit out of whack. I ran [p]target --joined-after 2017 50 01 and it saw that as a valid date, returning everyone in my server.
-Quote parsing is broken.
—You cannot search for users with a name/nick that has a quote in it.
—You cannot have a one-word argument surrounded in quotes.
" x" is valid, however "x " is not.
—I’m not particularly sure why the embed colors go 0: Red, 1-499: Green, 500-999: Orange, 1000+: Red.
-Commands can be used in DMs, however trying to target anyone raises an error:

"<datapath>\cogs\CogManager\cogs\targeter\targeter.py", line 317, in lookup
    matched = ctx.guild.members
AttributeError: 'NoneType' object has no attribute 'members'

-The command errors if the bot does not have permission to send embeds:

"<datapath>\cogs\CogManager\cogs\targeter\targeter.py", line 644, in target
    await ctx.send(embed=embed)
discord.errors.Forbidden: FORBIDDEN (status code: 403): Missing Permissions

Twenty
-The __author__ var does not have your current discrim (4227 vs 4779).
-You have a group command with only 1 subcommand in a cog that hasn’t been updated for 3 months. I doubt you are going to add more subcommands, so why a group command?
-You are unable to make a move in the same direction as your last move, even if that move is valid because of the last spawned number.
-If there are two valid sets in a row or column, only one is combined (2 2 2 2 becomes X 2 2 4).
-You might want to keep track of the player’s score (like in the actual game).
-The loss message is “On no…” instead of “Oh no…”.
-After the game is lost, the player still has to react one more reaction (that wasn’t the one they did last) in order for the game to end since the logic for that is after the wait_for.
-L31: start is defined yet never used.

UpdateChecker
-You do not have a class doc string, so [p]help UpdateChecker does not have a cog description.
-The warnings in your install message are wrong, the command is not [p]update all.
-You might want to make [p]cogupdater task a hidden command.
-You might want to revert what black did to your config identifier.
-Right now, if the channel does not exist the bot will message the owner that the channel does not exist, however it does not send the update message to the owner.
-The task will error if the bot does not have permissions to send embeds but embed mode is on. You might want to add a catch when sending the embed.
-The group command does not have any checks, so the command shows up in [p]help for all users.
-Trying to run [p]cogupdater all without downloader loaded causes and error:

"<datapath>\cogs\CogManager\cogs\updatechecker\updatechecker.py", line 204, in _update_all
    await ctx.invoke(cog._cog_update)
AttributeError: 'NoneType' object has no attribute '_cog_update'
#9

Thanks for reviewing my application @Flame442. I believe I have made all the necessary fixes, excluding things I mention below. If I have missed anything, please tell me and I will fix it as soon as possible.

Notes

Color:
As we figured out on Discord, you were importing from color, not colour, and it ended up working. In addition, linters have issue with this pip library, I’m not sure why. I believe all other issues have been fixed.

Commandchart
No notes here, I believe I fixed everything for this cog.

Cooldown
The arg parser was left over from a git issue, I had started implementing it, but git removed my changes in my core file, but kept the extra files. I removed them with the commit.

Deleter
It appears that the issues with the messages not being deleted when being spammed was because it was overwriting config. I implemented config locks to ensure they are added, and all of them were deleted for me after.

Editor
Now makes sure that the channel is a Text Channel.

Evolution
Warning was added into the cog install message, and fixed the hardcoded animal name of “chicken”. I had a spelling mistake in that fix, this was fixed in a later commit.

Listpermissions
Now allows for names, and uses pagify.

Maintenance
I believe everything here was fixed.

Minesweeper
Grammar was fixed, and now edits the board in one move, to prevent rate limiting and also now doesn’t message for invalid input. Warning was also added to the install message.

Simon
Now removes X emoji, added the score if they click it and now sequences build upon each other. I thought about what you said about how it may not work, but I didn’t want to delete it because it was already on my repo, so I chose to make it hidden.

SQL
Originally, line 1212 did not have a timeout, so it had no need for catching a timeout error, but I added a timeout and a handler. I had made this cog with the idea in mind for a way of storing data per-guild, but also making sure some other people can access it and others can’t, but without having to upload files or links or any other forms of sharing data.

SW
The all commands now sort by the ID if the results. Now uses ctx.typing() when accessing the resources. I talked with you on Discord, and since you said the name searching system wasn’t necessary per se, so I added it to my todo list.

Switcher
I did not put the DM only decorator because if the owner ends up running it in the guild, the bot gives no warning whatsoever. Sinbad advised me to allow it to be used in guilds, and give a warning if it is used there.

Targeter
Now allows for users to be displayed in a menu, and has --not-nick and --a-nick. I added extra checks to the date parser, and added a system to handle quote parsing. Now, if you wish to use a quote at the beginning or the end of the word, you can put \ before it and it will ignore it. I also allowed for one-word arguments with quotes to be that exactly, and fixed the " x" and "x ". However, I would like to note that I took this from the core Filter cog, and so an issue/PR should probably be made in order to fix this or put it on the list. Also, I changed it to use ctx.embed_color for colors, and made them guild only.

Twenty
The last move thing was me trying to fix another issue, but I thought of the wrong issue for some reason. I fixed that, and then correctly implemented the fix for the issue (spawning a new number when no numbers were moved). Now tracks user score, and also it didn’t actually wait for X to be pressed, it had just waited for the user to look at the board after the lost, but I changed it to auto-delete.

UpdateChecker
After time, I built something so that [p]cogupdater all is not needed, so I removed that command. Additionally, I’m not sure if the task would error if it can’t send embeds, since there actually is a forbidden check, but I added something to check for it anyway.


I believe I have addressed all the needed issues, besides the above. Again, please contact me if I forgot something and I will fix it as soon as I can.
#10

Howdy Neuro. The following issues popped up and need to be addressed.

Commit hash: e2216641afe6ab1d3ca7b2bed6f8deb2b8c42901
Red v3.1.1
Consider any unlisted cogs to need no changes

ListPermissions
-The new ID searching errors when running [p]listpermissions channel role <id> because results is not defined when using the ID.
-The new ID searching errors when running [p]listpermissions channel role ³ because ³ is valid under .isdigit(), but is not able to be int()ed.

Minesweeper
-Although the bot does not post an error message on an invalid input of minesweeper, it still deletes the message so it is still impossible to chat while playing the game.

SQL
-For some reason, [p]help Sql is throwing an error.
—I get a different error if I’m on 3.1.1 or 3.1.2, but both throw an error.
—As Sinbad pointed out, this is because you have a function called help in your class. Rename the function and use the name param of commands.command to call the command help.

Targeter
-Still needs a class doc string.

#11

Thanks for the update, fixed those with commit 0fc10171fca000022ffecbb4608ef5df9e764c6f.

#12

Thanks for fixing everything, Neuro. I am marking this as approved and will be granting you Cog Creator soon.

closed #13