[APPROVED] Tmerc-cogs

Discord Name:
tmerc#0013

GitHub Repository:


Description:
Miscellaneous cogs - some utilities, some requests, some server-admin type stuff.

Known Todos:

  • Real Readme
  • Port more cogs from v2

There are quite a few things here which seem to indicate not having kept up with things that have changed in V3. I’m not ready to give this a thumbs up, but I have not seen anything which isn’t fixable, and I’ve pointed out the immediate concerns which presented themselves below. Once these are handled, please update here, and I’ll take another look.

in your info.json files, author should be a list of strings. Additionally, these should be lowercased. (Reference)

(in multiple):

importing discord.ext.commands without specific purpose (and overriding later)

massdm:

swap manage_server for manage_guild in your check decorator.

_get_members_with_role isn’t needed anymore, discord.Role.members hold the list of members with that role. (Not a must change)

randimals:

L78: you’ve got no_pm=True (use commands.guild_only decorator instead) and pass_context=True which is true by default in V3.

streamrole:

You should probably just remove your own handling of send_help on command groups which dont do anything, and replace with pass as by default in Red, command groups invoke help now. if you’d like to retain manual control over this, use autohelp=False in the group decorator.

welcome:

same deal as some of the above with manage_server -> manage_guild and autohelp usage.

Thank you, this is sort of exactly what I was looking for :sweat_smile:

You’re right that I haven’t exactly kept up with changes, I fell out of keeping up for a while due to various things, but I’m hoping to be back and able to keep track again.

I’ll make the changes that you suggest, and post back again after I do so. Thanks again!

Okay, I think I’ve addressed everything that you pointed out. Embarrassingly, some of the issue were copy-paste issues from porting v2 cogs :frowning_face:. But I think all those, and the other things as well, are taken care of. If you could take another peek when you have a chance, I’d appreciate it!

Everything I brought up was addressed. I did miss something originally, unfortunately. With the version of aiohttp in use, session.close is a coroutine, and since __unload must be synchronous, this needs to use:

asyncio.get_event_loop().create_task(self.__session.close())

Considering the non-obviousness of this, and that this fails silently, I’m literally handing you the line here, and I’m also going to see what there is that can be done on the red side of things to improve this for the future.

Edit: Considering the scope of the particular last remaining issue, I’m giving this a :+1: in advance. The cogs do work even without this change, and while the issues on unload should be fixed when you have the time to do so, they are not user facing, and should not pose a large problem.

(to be clear, approved pending this fix, but no major issues with use prior)

Edit: issues were handled (This was noticed, but not commented on prior, still getting the hang of the process)