Migrate all message utils from bot and sir-lancebot.

[TizzySaurus]

As the title says, with approval from @ChrisLovering in dev-contrib.

This migration does contain a few changes, as per the following:

Constants

Since some of bot's message utils used constants, which don't exist on botcore, these had to be changed as follows:

• NEGATIVE_REPLIES - defined as a constant at top of file
• Emojis.trashcan - hardcoded into the wait_for_deletion parameter
• Roles.moderation_roles - defined as a constant at top of file

Since it was decided we didn't want the NEGATIVE_REPLIES or MODERATION_ROLES approach, but we don't know what we do want, I've instead just passed them in as arguments so that this can get merged (as per @mbaruh's suggestion).

Typehints

Since botcore runs on 3.10 (along with both bots), I changed all the Optional and Union imports to use the new | syntax (Optional[str] -> str | None and Union[str, int] -> str | int etc.).

Function Signatures

All the signatures remained the same, except for bot's wait_for_deletion, which now takes a bot parameter as the first argument.

This is because it used bot.instance.wait_for, which is now simply bot.wait_for.

Other bot.instance change

Similarly, the reaction_check util has user != bot.instance.user to restrict reactions to users who weren't the bot.

This has been changed to not bot.user instead, which is strictly different functionality but serves the same purpose.

Duplicate Utils

The only duplicate util was the sub_clyde util. Both implementations were the exact same, so nothing has changed in terms of functionality.

Imports

sir-lancebot had some from discord import ... imports, which have been changed to simply use the discord import, as well as some from discord.ext.commands import ... imports, which have been changed to use the from discord.ext import commands import.

I also changed both bot's from bot.log import get_logger and sir-lancebot's import logging to use from botcore.utils.logging import get_logger, and updated the log definition as such.

[netlify]

❌ Deploy Preview for bot-core failed.

Name	Link
🔨 Latest commit	ed8b4f7
🔍 Latest deploy log	https://app.netlify.com/sites/bot-core/deploys/63551e2d32ce99000957ff05

[HassanAbouelela]

I don’t think we should implement constants this way. How would someone test bot features which rely on these constants? To be clear, this is not your fault, nor something that can be fixed in this PR, as it requires serious work on how we approach constants in bot-core in general. Work that we have so far failed to do.

When bot-core was first starting, this topic came up, and I fought to try and get what I think is the best compromise implemented, however I met great resistance. I’ll start with the facts:

1. We have constants that are shared/duplicated between projects which are necessary for bot-core features. Proof is right here in this PR.
2. Not all these constants are easy to pass as function arguments (such as decorator arguments), and require especially complicated design to make work. Even for ones where we can pass them, it creates a pretty terrible interface that has a lot of pointless verbosities.
3. Since we already very rigidly define how the bots must be run, we can load files from the project directory and env file into botcore.

From here, things shift from requirements/facts to possible solutions. My suggested implementation is as follows:
Move the relevant constants directly into bot-core. Export those constants as a public interface to the bots. Bot-core itself would find those constants from files/env local to where it's running, which allows us to specify them as needed. This solves two major issues:

1. It reduces repetition, as a lot of the config (especially related to channels and roles and whatnot) is just the same IDs in different places.
2. It simplifies the interface for bot-core which can access the desired constants as it needs.

The largest pushback to this idea was from concerns that changing the constants would require two PRs, and to that I say: it's unavoidable. With our current approach, we have the constants hardcoded here, and coded into it's project which would require two PRs. If we pass constants to functions, we'd still have to change them in whichever project uses them. If we go with one of the other suggested ideas, which was to hardcode constant names into bot-core, and access them from the upstream project based on the names, that still requires two PRs, and makes bot-core a lot more fragile, and keeps the pointless repeition. If we use my suggested idea, you need to make two PRs, one to update the version in bot-core, and one to bump bot-core in the downstream project.

[mbaruh]

I agree that hardcoding these constants is the least preferable way to handle it. I still don't like the suggestion though.

If we pass constants to functions, we'd still have to change them in whichever project uses them

Yes, once. In this PR there is only one usage for these constants, and they could be easily passed as a bypass_roles argument or any other name you want.

I'd prefer bot-core to stay a generic library for bots. If we start tying it to specific projects I can only see it becoming cumbersome down the line. I'm also not sure how overriding those constants would work, but it sounds like it would be messier, and new contrib are already struggling with that.

And if providing a constant as an argument is not viable in some situation? Then that might be where bot-core's limit is, assuming we can agree on the same vision for it. It solves a lot, but it doesn't have to solve everything.

But for this specific PR again this can just be provided as a keyword arg.

[HassanAbouelela]

It is only one PR in some scenarios, but from my experience trying to migrate some of the utils, you'll still end up in a scenario having to change both. One example from here would be modifying the moderator role ID, and modifying the list of roles considered "mod roles" (like we did when we split the mod role in two).

In the provided example, you can work around that by passing the set of IDs directly, and despite being a cumbersome and repetitive API, will work. This is not true for all things, as our utilities make assumptions about what is expected.

I'd also like to point out that bot-core's setup would be very similar to python-discord/bot, and for most purposes we can do it with no changes required. The user will have a config file in the project directory they are working on, as we have now. That config file will contains the constants of bot-core (channels and roles and other URLs and IDs), and other project specific configuration. Or we could split the project configuration out. IMO this makes it even easier for contributors since you can just copy the same config into your different projects instead of setting up configs for each one.

I also don't like the notion that this is a "generic bot library" when that has been something we decided against multiple times, with a lot of the things here already functioning in ways that are unique to our bots.

[wookie184]

I like Hassan's suggestion here.

My understanding on roughly how it could work:

1. The project using bot-core defines the constants needed (e.g. with a pydantic config class). These would be based on it's own constants
2. This constants object is passed to the botcore Bot class on initialisation somehow.
3. The botcore Bot object sets these as global variables on a constants.py in bot-core (would need to be careful in exactly how this is done).
4. These constants can now be imported and used from anywhere in bot-core

---

I'd prefer bot-core to stay a generic library for bots. If we start tying it to specific projects I can only see it becoming cumbersome down the line.

@mbaruh

I'm not sure what particular case you're thinking of here. For something like wait_for_deletion, the fact it allows mods to delete messages is a feature of the util, not something specific to one bot or another. It's true that it's possible we may encounter a case where we want different overrides, but that's is the case regardless of if it's in bot-core on an individual bot.

I'm also not sure how overriding those constants would work, but it sounds like it would be messier, and new contrib are already struggling with that.

@mbaruh

I don't see how it would be different to what we have on individual projects. If a util doesn't do what you want you change it so it does or use something else. Where do we override constants on other projects and why would we need to do it here?

The largest pushback to this idea was from concerns that changing the constants would require two PRs, and to that I say: it's unavoidable.

@HassanAbouelela

What do you mean by "changing constants", like changing the value of a constant? If so why would that require two PRs? I don't think the values of constants should be stored on bot-core.

[mbaruh]

@HassanAbouelela

It is only one PR in some scenarios, but from my experience trying to migrate some of the utils, you'll still end up in a scenario having to change both. One example from here would be modifying the moderator role ID, and modifying the list of roles considered "mod roles" (like we did when we split the mod role in two).

I said in the previous comment I agree hardocoding is not a good solution, so I'm not sure what this is aimed at.

In the provided example, you can work around that by passing the set of IDs directly, and despite being a cumbersome and repetitive API, will work. This is not true for all things, as our utilities make assumptions about what is expected.

I'd like to understand what utils use such assumptions.

we could split the project configuration out. IMO this makes it even easier for contributors since you can just copy the same config into your different projects instead of setting up configs for each one.

I'm really not sure about this. We have the bot config, the optional metricity config, and we're also considering another config for the automatic server creation tool. With another split, that's potentially 4 configs for a single project, it sounds terrible.

I also don't like the notion that this is a "generic bot library" when that has been something we decided against multiple times, with a lot of the things here already functioning in ways that are unique to our bots.

bot-core is based on common usage cases in our bots, so of course it's going to have things that are specific to our bots, but they can be easily used in any project which imports bot-core. I think keeping it clean of constants will help decide what is and isn't appropriate for this project. Otherwise bot-core can just be a dump of anything that is similar between the bots, and it will just help create situations where later we find out that the we moved a util away from bot A but now we need the util to work in a way that doesn't reconcile with how it was written into bot-core. I'm also not sure what we decided multiple times against since I'm not suggesting publishing bot-core on PyPI.

---

@wookie184

I don't see how it would be different to what we have on individual projects. If a util doesn't do what you want you change it so it does or use something else. Where do we override constants on other projects and why would we need to do it here?

What I'm trying to say is that we will need to either

• Have another config
• Override the constants in the same config without changing anything, but some of the defaults will actually be loaded in bot-core and are not visible in the bot's code itself, which IMO is just confusing.

What do you mean by "changing constants", like changing the value of a constant? If so why would that require two PRs? I don't think the values of constants should be stored on bot-core.

That's the whole suggestion though, storing the common constants in bot-core.

[TizzySaurus]

That's the whole suggestion though, storing the common constants in bot-core.

Isn't this the whole purpose of bot-core (to remove duplicate code between the bots)?

I personally would be for a constants.py file on bot-core, that has all the constants that are required on both bot and lancebot. Other constants can stay within their respective repo (as they already are).

Yes, this means 2 PRs when moving constants to bot-core (1 on bot-core and one on the repo it used to be on for migrating to bot-core imports), but I don't see this as a huge issue, and is already the case for all the utils etc. we have migrated in the past, so is no real difference to what we've been doing this whole time.

[TizzySaurus]

Since we aren't much closer to an actual conclusion with constants, I've gone with @mbaruh's suggestion of just making the constants arguments inside the respective functions: 7c4060e.

[ChrisLovering]

Further to this, before we can merge this, we should have a bot PR that implements this logic, so we can actually test the code.

[TizzySaurus]

Further to this, before we can merge this, we should have a bot PR that implements this logic, so we can actually test the code.

Yep! I'll work on a PR for both bot and lance migration to these utils. 👍