Introduce subfolders into the cog folder so that we can organize the cogs better.

[pydis-bot]

Originally posted by Leon Sandøy:

We should consider either consolidating similar cogs into a single cog with more features (like an Admin cog that had big brother, clean features, and defcon), or to create subfolders in the cogs folder so we can at least group the similar cogs together. The folder is growing unruly.

[pydis-bot]

Comment from Johannes Christ:

I disagree heavily! Every cog should do one thing, and do that thing well. Grouping together too many commands in one cog will make it harder to maintain, and in addition to that, it makes it harder to find commands.

To support my argumentation - Without looking at the source code, and trying to put yourself in the shoes of a new contributor, answer the following questions:

Where does the code block sanitizer live?

In the Bot cog. In my opinion, the bot cog should contain commands related to the bot, and this should be in its own cog.

---

Which cog handles the subscription to announcements?

The Verification cog. This makes no sense to me at all. The only reason that this has a place there might be because it is mentioned in the doc string, but that still doesn't make it a valid spot in my opinion. If anything, we should create an extra cog, named Notifications or similar, which we could also expand later to allow subscribing to other things than regular announcements, if applicable.

---

Which cog updates users on the website?

The Events cog. This makes no sense to me - the name implies it's handling general events. It currently handles command errors, and that event. This doesn't have any place there. This is not something general, and, in my opinion, should be factored out to another cog, e.g. UserUpdater. In addition to that, the cog name currently makes it sound like it handles all events. That is not the case.

---

A couple other concerns:

• Merging cogs will not only make the cog class itself more unreadable, it will also add more imports and potentially more converters and other utility functions, hurting maintainability of these single files even more.

• Unloading a single cog will have a way heavier impact. Taking your example:

  an Admin cog that had big brother, clean features, and defcon

  What happens if we discover a critical bug in just one of these commands? For example, the big brother cog flooding the channel and making the bot hit ratelimits, unable to respond? Purely hypothetically, what if it's 4 AM Europe time, lucy is the only one online, on her phone, the server is being raided, and Karl the Great (hypothetical contributor name) accidentally put a critical bug in the message clean command that allows all members to use it? What happens then? Will lucy unload the Administration cog to block access to the vulnerability, but effectively losing all access to the moderation commands? Will lucy not unload the cog, but at the same time allow people to use (the hypothetical) clean channel command, allowing everyone to delete messages through the bot in every channel without any (sane) limit? Will lucy go to sleep (that, certainly not)?

  Now, imagine the whole jazz above again but with the clean command being in its own cog, like it should be (because there's many subcommands, clean <amount>, clean user, clean bots, clean hard drive, whatever). One could easily unload the clean cog while leaving on access to every other command in the bot.

  Grouping based on functionality is great, but clumping based on functionality isn't.

• The ClickUp cog is unused. It's just sitting there. It's dead weight. We can remove that

As another example, in my bot, every command is its own file. This is very simple to work on, compared to e.g. the Snake cog.

[pydis-bot]

Comment from Leon Sandøy:

you've made many fine arguments towards why we shouldn't do my first suggestion, and I buy them. What about my second suggestion?

[pydis-bot]

Comment from Johannes Christ:

My bad, I have not read that yet.
I think the amount of cogs is currently fairly low and we don't need it. It's certainly a good idea for later.

[pydis-bot]

Comment from Leon Sandøy:

maybe you're used to it since you have "one cog for each command", which sounds absolutely insane to me. With the clean cog I'm working on, we'll have 22 cogs. I think when you have 22 files in a folder, it's time to start making subfolders. anyone else have an opinion? could it really hurt to be more organized?

[pydis-bot]

Comment from Kingsley McDonald:

i believe that subfolders is the way to go. i like the unix philosophy, in which the following is stated:

Make each program do one thing well. To do a new job, build afresh rather than complicate old programs by adding new "features".

[pydis-bot]

Comment from Leon Sandøy:

after volcyys 2000 word writeup, I agree that folders seems like the better option. he makes a bunch of valid points about separation and I agree with all of them. I do think the need for those folders is there already, though.

[pydis-bot]

Comment from Johannes Christ:

mentioned in merge request !29

[sco1]

So what's still outstanding from this discussion. It seems like consolidation was agreed against, while adding some more structure to the cogs folder was agreed upon?

[jb3]

I think that this can wait for us to split up the bots and then we can re-evaluate which bots may need their cogs organising even more than that.

[lemonsaurus]

We're not doing the splitting up the bots thing after all, it seems. So we can probably go ahead and do this.

[mathsman5133]

I had a good think about this and sensible ways of dishing up some of the current files into subfolders.
(Names might need a change)

• Automod: All things that trigger bot messages/reactions without any command - namely antispam.py, the on_message and related parts of bot.py, filtering.py, and token_remover.py. Maybe there's more but that's where I'd start

• General commands for all to use (that aren't bot, server or site info): doc.py, free.py, reddit.py, reminders.py, snekbox.py, tags.py, wolfram.py

• Bot/server/site info: part of bot.py, information.py, help.py, site.py

• Bot admin/settings - cogs.py, defcon.py, error_handler.py, eval.py, logging.py, off_topic_names.py, verification.py.

• I think clean.py is a moderation tool and thus be moved to moderation subpackage

• I don't know why security.py exists, can easily be put in __main__.py

• We could start a new folder: Events for either event info commands or specifically jams.py

• That leaves utils.py, and aliases.py (and anything else I missed)

An easy way to implement this in both the help command and reloading of cogs?
I think in a class, a Category or Package or Subfolder whatever the name would be helpful.
In __init__.py of the category:

import cog1
import cog2
import cog3

from blah import Category

def setup(bot):
    moderation = Category(bot, name='moderation', description='cool commands to ban people')
    moderation.add_cogs(cog1, cog2, cog3)

A few ways this could be used:

• In help:

category = bot.get_category(category_name)
embed = discord.Embed(title=category.title, description=category.description)
for n in category.cogs:
    embed.add_field(name=n.name, value=n.description)

# or if you wanted commands for a category,
commands = sorted(category.commands, key=get_category_name)

• In reloading/loading of extensions:

!reload category

bot.remove_category(name) (needs a new method in subclassed bot)
# or maybe just 
bot.get_category(name).remove()

[scragly]

@MarkKoz and I recently defined groups for the current bot functionality in order to define easy enough to understand area labels in the Github bot repository.

We settled with the following:

Filters

• antimalware
• antispam
• filtering
• token_remover

Information

• doc
• free
• help
• information
• reddit
• site
• tags
• wolfram

Moderation

• moderation
• defcon
• verification

Utility

• bot
• clean
• eval
• extensions
• jams
• reminders
• snekbox
• utils

While we've listed the extensions like that currently, we are going to refine the structure of the extensions themselves where appropriate to make it as logical as possible. For example, bot contains the embed utility command and the about info command, so they should stick to different groups instead.

Extensions like security and error_handling isn't listed here as they are backend group items and should belong in the core of the bot.

[MarkKoz]

This list isn't final even though I'm pretty happy with it personally. Feel free to comment if you have any suggestion improvements or changes to it. Furthermore, I think we should take the opportunity to rename the `cogs` subpackage to `extensions` or probably the shorter, and likely better, `exts`.

[scragly]

I think we should take the opportunity to rename the cogs
subpackage to extensions or probably the shorter, and likely better,
exts.

definitely agree

[MarkKoz]

I think we're waiting for the PRs to clear up before implementing this to avoid many conflicts. Besides that, there's nothing really else stopping it. I am happy with the discussion and conclusion reached for the directory structure.

[lemonsaurus]

@MarkKoz Any specific PRs we're waiting for? I feel like this is gonna cause conflicts no matter when we do it, because we're never gonna have 0 PRs.

Also, I like the suggested subfolders. I'm not sure I like exts very much. extensions seems fine, but I don't really get why cogs is so bad.

[MarkKoz]

@lemonsaurus

Any specific PRs we're waiting for?

No, not really. Perhaps the help channel system. Anyway, I can just take responsibility for resolving conflicts on all PRs myself.

I'm not sure I like exts very much. extensions seems fine, but I don't really get why cogs is so bad.

In a sense, "extensions" is more correct since the package contains modules which Discord considers extensions. In turn the extensions contain the cogs. So the package contains extensions, not cogs. That was the original line of thinking. However, in retrospect "cogs" is not so bad nor is it really incorrect. The package does contain cogs, just not directly.

That being said, seasonalbot has switched to "exts". Should this repo should mimic it anyway?

[ks129]

My idea for the new directory layout that applies Scragly's sorting:

bot/
    extensions/
        filters/
            __init__.py
            antimalware.py
            antispam.py
            filtering.py
            token_remover.py
            webhook_remover.py (added by my recently, but should be here)
        __init__.py
        security.py
        error_handler.py

I didn't included every category currently, but this should how I see this. __init__.pys should hold setup functions, so loading should like bot.extensions, bot.extensions.filters.

[lemonsaurus]

@MarkKoz alright, and besides your argument about how it's technically more correct to use extensions than cogs, it also makes it clearer to users unfamiliar with the term cog and with discord.py what the folder contains.

consistency with seasonalbot would be nice, I guess. I'm just generally allergic to abbreviated names for things. I think extensions is better than exts and wish we had the former on both repos, and with python-discord/sir-lancebot#329 recently merged, there's no better time to do it than right now. Especially if @kwzrd did it, we'd lose no blame at all.

[lemonsaurus]

hmm, these imports with from bot.exts... look familiar. Is exts used in discord.py or something? If there's an established use for this, I'd be more inclined to agree we use it.

[ks129]

@lemonsaurus discord.py have commands and tasks under ext (from discord.ext import commands)

[kwzrd]

Hey, yea I decided to go with exts in SeasonalBot. I was looking for something to replace seasons (as the extensions within no longer bind to seasons), and exts seemed the most appropriate. As @MarkKoz mentions, it is technically more correct than cogs (plus I really don't like the word cog), and I personally find extensions unnecessarily long and verbose.

As @ks129 notes, d.py uses the terminology although they drop the plurality. I personally think it's better in plural, but I suppose that's a very personal preference.

[lemonsaurus]

in plural it's less ambiguous - ext could be external, but exts has fewer possibilities. extensions has the fewest of all. I don't like ambiguity, but I guess we'll go with exts everywhere then.

[SebastiaanZ]

I agree with @lemonsaurus. My preference would have been extensions as I don't really like abbreviations when they're not really necessary, but consistency is also important. So, let's go with exts in that case.

[ks129]

About setup functions, should these in __init__.py files or in each cog file?

[scragly]

Within __init__.py

[MarkKoz]

Here's what I currently have:

exts
├── backend
│   ├── sync
│   │   ├── __init__.py
│   │   ├── cog.py
│   │   └── syncers.py
│   ├── __init__.py
│   ├── config_verifier.py
│   ├── error_handler.py
│   └── logging.py
├── filters
│   ├── __init__.py
│   ├── antimalware.py
│   ├── antispam.py
│   ├── filtering.py
│   ├── security.py
│   ├── token_remover.py
│   └── webhook_remover.py
├── info
│   ├── __init__.py
│   ├── doc.py
│   ├── help.py
│   ├── information.py
│   ├── python_news.py
│   ├── reddit.py
│   ├── site.py
│   ├── stats.py
│   ├── tags.py
│   └── wolfram.py
├── moderation
│   ├── infraction
│   │   ├── __init__.py
│   │   ├── infractions.py
│   │   ├── management.py
│   │   ├── scheduler.py
│   │   ├── superstarify.py
│   │   └── utils.py
│   ├── watchchannels
│   │   ├── __init__.py
│   │   ├── bigbrother.py
│   │   ├── talentpool.py
│   │   └── watchchannel.py
│   ├── __init__.py
│   ├── defcon.py
│   ├── modlog.py
│   ├── silence.py
│   └── verification.py
├── utils
│   ├── __init__.py
│   ├── bot.py
│   ├── clean.py
│   ├── eval.py
│   ├── extensions.py
│   ├── jams.py
│   ├── reminders.py
│   ├── snekbox.py
│   └── utils.py
├── __init__.py
├── alias.py
├── duck_pond.py
├── help_channels.py
└── off_topic_names.py

I'm not satisfied with the name for the infraction sub-package and the names of the modules within, but they're decent. Whether clean should be in utils or moderation is also up for debate.

---

Each sub-package could be treated as an extension, but this limits control over which cogs are loaded. On the other hand, if each module is an extension, it's difficult to distinguish normal modules from extensions without at least importing them. This is relevant for the functionality of the extension load, unload, and reload commands. Which way should we go with?

Perhaps each module could also be an extension. The sub-packages could be "meta extensions" that load the extensions within them. setup() isn't limited to add_cog(); it can also do load_extension(). However, I am unsure of how the "meta extensions" would deal with extensions that are already loaded (or unloaded, if trying to unload the "meta extension").

[kwzrd]

Each sub-package could be treated as an extension, but this limits control over which cogs are loaded.

I just had this thought earlier today and wanted to comment here, but I see you've already been thinking about this. I've been finding it a little awkward to not be able to reload a specific cog in the moderation package, for example, and wanted to propose that each cog is its own extension to alleviate this. The inability to unload a specific cog while keeping others intact seems like a much bigger problem, although one that we haven't run into yet.

However, you're also raising some good points about non-extension modules. Perhaps they could just be nested further into a helpers sub-package, or similar? I'm not sure, but I feel like this is something we should be talking about.

[MarkKoz]

We effectively manually maintain a list of extensions when we load them in __main__. I haven't heard any complaints and I like the explicitness of it. We can turn this into an actual list that the extension management cog can use to know which modules are valid extensions to load/unload. It's based on the assumption that we want to load all extensions by default. If not, separate lists would be required.

This avoids any complicated extension detection code at the cost of being vulnerable to contributors forgetting to update the list. However, that doesn't seem like a valid concern since forgetting to load the extension would be a very obvious error to catch; it hasn't been a problem so far.

Perhaps they could just be nested further into a helpers sub-package, or similar

This would be OK when there are many modules, but would just feel clumsy if there's one or two. I don't completely hate the idea, but I want to keep exploring other solutions, such as my above proposal.

[MarkKoz]

New idea: prefix names of non-extension modules with an underscore.