Discord Name:
Neuro Assassin#4838 <@473541068378341376>
GitHub Repository (Must be V3):
I don’t feel your repo is currently ready to be listed under the approved repositories. I’d be happy to re-review this when you have more to show here.
(for each of your) info.json’s the keys should all be lower case. Additionally, the author key should be a list of strings. (can be a single element list)
The warning you included in the cog doc string for simon needs to also be in the install message of the cog. (part of info.json)
Outage report, there isn’t a good amount of user feedback in the cog. The help for it indicates it can detect if a site or a company’s service is down, but the way it is implemented, if you try to check a site, it will always fail. Checking a company works some of the time, but still doesn’t actually provide good user facing feedback for this. (as an example, providing youtube works, but, youtube.com and https://www.youtube.com result in a 404. )
Additionally, 2048 and simon feel incredibly limited as cogs, as both of them only allow a single user to interact with the cog at once.
Unfortunately, I don’t have a great answer for how you can improve the appeal of simon. I understand and agree with the decision to handle it the way you did to avoid rate limit issues. 2048 is in a similar boat.
As for the outage detection, If you’re looking to improve this, I’d try to find a good way to normalize user input for it. That said, it also appears to be a very limited source site, so you may even want to use something like a custom converter which limits it to known services of the site.
Thanks for getting back to me on it. I’m working on editing it right now. Also, would filtering the inputted string for outage report to remove ".com"s and other suffixes and removing "https://"s work for it? Thanks again.
After our last discussion via DM in discord, I’m marking your application as “on hold.” Feel free to bump this once you come up with solutions for the problems discussed.
I received a request to give this another look (via discord DM). I’m moving this to a definitive, but not indefinite “No”. There is not enough up to current standards to put this in the approved cog repository list at this time. At such time when that has changed, you may reapply.
I’ll note a handful of things below which were noticed as I was going through what has changed since being added (as well as noting a few things which hadn’t)
Let’s start with the previous things left unaddressed:
Twenty and simon both still work, and both are accompanied by warnings about potential ratelimits, but neither were changed to address the issue of multiple games for what are ostensibly to be used by multiple people (otherwise, why discord, instead of a dedicated app?
webstatus
This is not what I meant with needing to handle that. While it technically does in most cases, you’re done this in an incredibly inefficient manner that still doesn’t address that the site you are using for this does not work with arbitrary sites, but a specific list of services.
This is a good case for working with urllib or a regex, then confirming the service inquired about is included in what are tracked on outage.report
Okay, with that said, here’s what I noticed in what is new:
namechecker:
Help for this is incredibly unhelpful:
missing docstrings, shadowing the python builtin set in the settings command.
Ambiguous user facing variables in command signatures.
Attempts to match things which cannot exist in names:
https://discordapp.com/developers/docs/resources/user#usernames-and-nicknames
(you’re attempting to match http:// or https://)
Method of matching is poor for the intent, and is potentially paired with automated kick or ban.
This cog with just set to “auto” paired with an action of “ban” would ban people with any of the following names:
“.com bubble burster”, “Free Kim .com”, “polka.organizer”
None of which match the intent (a url in the name).
It’s also matching a fixed set of strings by iterative comparison to member.name.lower() and does not cache the .lower() per iteration.
This is again, another good case for regex use.
every time you look at an action type, you are using .lower() and a string comparison, instead of storing it lowered to begin with.
You should use the built-in handling for boolean types in the commands extension for true/false handling for consistency with other commands in core red.
commandchart (sigh).
This is a modification to someone else’s cog. You acknowledged that, and I don’t have a problem with that, but the modifications here are not done well enough to hold up to scrutiny.
ctx.clean_prefix
This entire block can be replaced with:
async for msg in channel.history(limit=number, after=datetime_object):
message_context = await self.bot.get_context(msg)
if message_context.valid:
message_list.append(message_context.command.qualified_name)
And it would be both more accurate, and faster.