Commit hash: bbcb02e9800781e9917c430d74a942afa2fd0543
Consider anything prefaced with “You might want” to be optional
-I initially attempted to use
[p]clash settoken in DMs with the bot, this errored with the following traceback. Ideally token setting commands should only be used in DMs, so the fact that it does not work at all in DMs is a problem.
Traceback (most recent call last):
File "cogs\CogManager\cogs\clashofclans\clashofclans.py", line 577, in settoken
AttributeError: 'NoneType' object has no attribute 'me'
-Commands which ask you to "enter a valid clan tag or link your clan using
[p]clash linkclan" should ask you to use the actual command
-The “enter a valid clan tag” message is sent from
[p]clash army, however this command expects a player tag not a clan tag.
-Running any command that takes a clan tag with an invalid clan tag causes the command to error. This happens because
check_response_for_errors returns 404 on 404 response code, but this function is called as
if not await self.check_response_for_errors(response):, which treats 404 as True-like (non-zero int). You appear to handle a value of 404 in your commands instead of in
[p]account link with an invalid tag errors, but still “links” that value. As such, running any of the commands which use this linked value will error as well.
-For some reason
[p]clash clan works with a passed clan tag, but errors with a linked clan tag.
-I’m not really sure why you can link with multiple accounts when only one will be “used” by other commands. You might want to only allow linking with one account at a time. EDIT: after reading the code, it seems like this is to allow the user to pass a single
int to select that “index” of account. This isn’t explained anywhere in the cog and doesn’t work past 9. EDIT2: There is a command
[p]account list, however it doesn’t explain this feature and is hidden until an account is added (however it allows passing in another user via an argument, and handles them not having an account set up?). Overall I still think this “feature” leads to some poor UX as it stands right now, so you should probably either make it more clear to the user how it works or remove it.
-If you are going to require a linked account to run
[p]account linkclan, and
[p]clash player can output the player’s current clan, you might want to just dynamically fetch the clan value for the linked account rather than adding an extra step to link the clan.
[p]clash resettoken does not check if the bot can add reactions.
-None of the commands which send embeds check if the bot can send embeds.
-You might want to check permissions / hierarchy first instead of blindly trying to add roles to users and catching the exception if
-You don’t handle deleted roles in your
[p]sr list or
-When adding a new role, you need to compare the user’s highest role to that role to prevent permissions escalations. Right now, users with the
manage_guild permission can allow themselves to get any role higher than their highest role but lower than the bot’s highest role.
install_msg directs users to
[p]wordcount, however the only valid command is
[p]wordcountset channel with no channel param and no existing channel causes the command to error.
-You might want to fix the “count failed” message as it seems to be printing a tuple right now. When you are expanding a string into several lines with
(), you don’t need to add commas at the end of each line (L149-151 and a number of other places).
[p]wordcountset resetcount and
[p]wordcountset setcount throw an error trying to say they succeeded.
-You might want to make
ignorefailed when a user attempts to recount. Right now the count will reset when a user attempts to say a second word even if
ignorefailed is enabled.
-L130, I believe that “hundread” will cause issues if people get to that high of a count.