[APPROVED] Maxcogs

Applications
#1

Discord Name:
MAX#1000

GitHub Repository (Must be V3):

Description:
This repo has 6 cogs + 4 cogs that are API based. Fun, general and utility cogs.

#2

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.

#3

Hello.
I confirm that it’s ready to review. all cogs should be ready on 3.5 and are on master.

#4
#5

Application Review - maxcogs

Thank you for your patience whilst I reviewed your repo.

Commit hash used for review: cd0ec76ed7c4f8a9e4a0ac43bc39a275b5afc00e

Additional changes made after this commit will be reviewed in conjunction with the fixes made from this review.

General comments

Your code is structured and commented really well, making for a much more efficient review. Steps within the code are concise and do not do anything unnecessary. The documentations for your cogs are also well structured. I was also pleased by how you disable settings within listeners if the current context does not comply with permission rules or other conditions.

I really enjoyed making and testing this repo, as the cogs are very easy to use. On the whole, there isn’t much to change other than a few optimisations and grammar corrections.

I have some recommendations on how to write code a little more efficiently in the future, or when adjusting features. When writing code in the future, it would be nice if you use annotations more to help yourself, external contributors, and your type checker. Additionally, there are a few locations where the code becomes slightly confusing from a readability point of view due to deceiving variable names. For example, inside embed.py, the function embedgen appears to take url as one of its parameters, but it appears to be a dictionary when on first thought one would think url would be a string. Whilst it seems like it doesn’t matter too much, it could cause errors, especially without the presence of annotations.

Cog Reviews

  • :pencil2: Required change
  • :person_shrugging: Optional change (not required for review)
  • :bulb: Tip (not required at all unless you wish to go ahead with said tip)

AutoPublisher

NekosBest

  • :person_shrugging: embed.py - This file’s functions take self, which appears to be the instance of the cog every time. This means that you could instead put these functions inside the cog’s class, to prevent importing them instead.

  • :person_shrugging: embed.py (line 40-43) - Instead of indexing the original dictionary multiple times, you could define a result variable (url["results"][0]), and just index that variable for each field you want (artist_name, source_url, etc…).

    # Instead of this...
artist_name = url["results"][0]["artist_name"]
source_url = url["results"][0]["source_url"]
artist_href = url["results"][0]["artist_href"]
image = url["results"][0]["url"]
 
# Try this:
result = url["results"][0]
artist_name = result["artist_name"]
source_url = result["source_url"]
artist_href = result["artist_href"]
image = result["url"]

NoSpoiler

RolePlayCog

General comment: I don’t think there should be multiple references to how the MemberConverter works in this cog’s docs.

  • :pencil2: roleplaycog.py (line 34) - Logger names should conventionally follow the appropriate format as stated in the cog creation docs (red.your_repo_name.cog_name). The veryfun part will need to be replaced with roleplaycog.

    log = logging.getLogger("red.maxcogs.roleplaycog")

  • :person_shrugging: roleplaycog.py (line 111) - There is no real reason to use the async context manager here, as we aren’t updating values inside Config. Instead, we can just define config on one line.

  • :bulb: roleplaycog.py (line 177) - If this bothers you, you could consider putting all of the roleplay commands inside the roleplay group. Then you can have more freedom with aliases.

  • :pencil2: roleplaycog.py (line 246) - “Blushs” → blushes

  • :pencil2: roleplaycog.py (line 258, 276 and 282) - For these docstrings, I feel they are grammatically incorrect and that “a” should be replaced with “at a”. For example, “pout at a user” instead of “pout a user”.

Tcgcard

  • :pencil2: tcgcard.py (line 14) - The cog should use title case, as users expect this in order to receive help for the cog. You should consider renaming the class to “TCGCard”, as TCG should stay capitalized as it’s an acronym. I suppose “TcgCard” would work, though.

  • :person_shrugging: tcgcard.py (line 91, 97 and 98) - These str() calls are redundant seeing as they’re already supplied as strings from the API, and are guaranteed to be keys that appear in the returned JSON.

WhosThatPokemon

General comment: Might it be worth revealing the name of the pokemon if they are unable to submit a correct answer within the timeframe?

  • :person_shrugging: view.py (line 51) - “Guessed” should be lowercase here.

  • :pencil2: whosthatpokemon.py (line 152) - The parameter generation isn’t quite working here when executing text based commands, because the converter’s exception is not being shown to the user. This is because how Optional works, using Optional[Converter] means that if the parser were to fail (in this case, raise BadArgument), the default from the parameter will be returned (in this case, None) or None anyway, and the exception will not be sent as a message. See here for more information.

Final Comment

I see you’ve added an additional cog whilst this review has been happening. I will review that in conjunction with your changes from this review. Thanks.

#6

So i’ve applied most of the changes from you in eeac5ce09ddb3d790644aeb9ef321fc1e6397694.

A little note that i removed the new cog you mention there. + you’ve looked at a little to old codes, some of those codes you’ve looked at were removed previously in the code such as the stats command in the roleplaycog.

Attitionally to the WhosThatPokemon, i couldnt get it to raise BadArgument but instead went around the corner with a if generation is None: which seemsed to work perfectly with no matter how i were typing, if it were eeeeee or gen43455 example, it’d say a message below there, but if this has to be changed, to use badargument instead, feel free to notify me again about it and ill try again, i couldnt understand the examples.

Anyother addional with the Tcgcard cog, i did change to TCGCard, though i am unsure what you mean with the secoud line there with the api changes, i havent really touched much with this cog since i got it transferred to my repo.

hope this helps and hopefully i did most of the changes correctly.

#7

Thanks for making those changes. I need you to address the following points before I can mark this repository as approved:

  • nospoiler.py, (line 143, 145 and 148) - The command now errors due to a NameError:

    "nospoiler\nospoiler.py", line 143, in toggle
    enabled = await self.config.guild(guild).enabled()
NameError: name 'guild' is not defined

  • whosthatpokemon - Unfortunately your workaround isn’t completely right yet, as your handling means that the generation parameter is no longer optional, whilst the docstring still states

    You can optionally specify generation from gen1 to gen8 only,
    to restrict this guessing game to specific Pokemon generation.

    Otherwise, it will default to pulling random pokemon from gen 1 to gen 8.

#8

Done with nospoiler 314bb68427a8632730a297dbe81c8ef3720c9c30

Also whosthatpokemon has been fixed in 7412e13a83e4bd5c9092757d2b603e455d2a38f8

#9

Excellent, thank you for making those changes. Marking this as approved!

1 Like
#10