Hopefully got everything fixed this time around.
Discord Name:
Wyn#0004
GitHub Repository (Must be V3):
Description:
Fun/utility cogs I’ve made or ported for V3.
Howdy! Sorry for the delay in reviewing your application. We at QA have been working on finalizing some documentation outlining exactly how these applications are reviewed. Since this application was made before the documentation was live, I am letting you know so you can double check that your repo meets these new requirements. We hope this transparency will help you to better prepare your repo for review.
Will check it out.
Hi Wyn, thank you for your patience while I review your repo.
Commit hash at time of review - 41b604c8094e1fdc488f103b7e858a3f6ffee9ad
A lot of my comments are suggestions or things that you should be aware of that don’t especially require changes, but they are suggestions for better user facing messages or interactions. There are a few points that need to be addressed and I have phrased them as absolutes.
Repo level info.json
You may want to add the Cog Server invite to your repo add message. “For issues, bugs, or suggestions contact me in Reds discord” -> “For issues, bugs, or suggestions contact me in Red’s 3rd party cog Discord at https://discord.gg/GET4DVk in the support-othercogs channel”
lyrics
lyrics.py
As with all audio add-on cogs, I have to warn that the methods you are using to interact with the audio player via lavalink directly will change at some point to be an actual audio API. I don’t have changes for you regarding this, but know that this implementation can be fragile.
[p]lyrics search and [p]lyrics spotify are not working for me on one bot that is receiving a Google captcha instead of the site page scrape (it does work on other vpses). The error message was a bit confusing to me, you may want to reformat that if possible (line 201). Alternatively you could catch the captcha text from the BS page scrape - let me know if you’d like to know what the captcha page response looks like.
On line 107 you check for embed permissions after the fact, this check could be done via a command decorator or earlier in the code to return before executing.
info.json
You may want to change the wording on the install_msg to say “Type [p]lyrics for more information.” -> “Type [p]lyrics for more information after loading the cog.”
You should add these permissions requirements to your info.json as well. "permissions": ["add_reactions", "embed_links"]
anisearch
LICENSE
I see that Anisearch was changed to AGPL from GPL on Jintaku’s version, but that was in October of 2018 when you made your port earlier in April of that year. The GPL license you included for this is correct based on the history, otherwise I would be asking you to use AGPL instead.
anisearch.py
Consider using more descriptive variables for each command’s entered words. For example, on the character command, the character name variable to search for is named entered_title when it could be something like character_name.
You must check for embed permissions and add reaction permissions on your commands as you do not currently. This can either be done via a decorator on the command or you handling the perm check in the code with a user friendly message.
info.json
You should add these permissions requirements to your info.json as well. "permissions": ["add_reactions", "embed_links"]
sysinfo
LICENSE
Sysinfo by Ritsu looks to be unlicensed and you have added a GPL license to your port. You should speak with Ritsu/Kagami if possible (they are in the Red servers) to get their permission to port their work and add a different license to it. Also please take a look at the note under the sysinfo.py header below regarding the BSD original source of some scripts. This should be resolved before I can approve this repo.
sysinfo.py
The attribution to https://github.com/giampaolo/psutil/tree/master/scripts for “most of these scripts” led me to superficially look into the source which is licensed under BSD 3-Clause and not GPL or Unlicensed.
On the help for [p]sysinfo info, it displays other args you can use for other information, like boot. The help text examples for these args need to have “info” added. For example right now the help text says sysinfo boot Shows boot time but this should read sysinfo info boot Shows boot time. Also on that command it has [args...] which should be limited to one argument instead of hinting that it takes multiple, which it doesn’t, it only accepts the first one right now.
nyaa
nyaa.py
Needs a cog/class-level docstring.
Not quite sure what “User arguments - Show name” means in the context of the [p]nyaa lookup command help text. There doesn’t seem to be options for user arguments. If there are, please provide some example queries in your docstring or another way of letting users know the command options.
You have a bare exception on line 93, while this is a nice catch-all usually these are frowned upon.
General comments
I couldn’t find any information regarding licensing on your other two cogs or what you use on your repo. You may want to make this available, possibly by setting a GPL license for your repo (or whatever other license you’d like), and then continuing to provide the other licenses for other cogs within the cog folder if they are different, as you are already doing. By leaving your repo without a license technically users are not even allowed to copy the repo, let alone install cogs or operate the code, but I am not a lawyer so take all this with a grain of salt.
Possibly consider leveraging the cog disabling API for Lyrics.
Thanks Aika, will get on asap.
Hi Wyn, thanks for your updates. I found something funky with lyrics while going over your changes but it is not needed for this review to pass and I can talk about it with you elsewhere. You should see your role, etc in the Red servers in a bit when we get that all settled.