[Tracking] Discuss merging Bank/Economy by 3.3/3.4

[Drapersniper]

As discussed on discord, opening this issue so that the community, cog creators and contributes can voice their opinion/thoughts on this consideration.

The idea of merging these 2 core cogs was brought up on discord, #2961 already does a bit of ground working however that PR is only reorganizing/renaming some commands.

We would like to invite everyone to have a say on why the feel it should/shouldn't happen.

Personally I think the merge would be beneficial, moving all bank commands into Economy and have Economy be the standalone interface for both currency and the built in method of interacting with the bots economy.

At the moment the only purpose of the Bank cog is to provide an interface. The commands at the moment are [p]bankset, [p] bankset toggleglobal , [p] bankset bankname , [p] bankset creditsname and [p] bankset maxbal my proposal would be to move there into the Economy cog and rename the redbot.core.bank to redbot.core.economy

Another benefit of this in my eyes is that it gives the sense of more "complete" cogs for cogs that are part of core, aside from bank all other core cogs are full of features, and the state of bank and economy always been odd as for economy bank is always needed if the user wants to setup basic balances, currency name which are part of the wider bot economy.

On the other side of the coin this would be a massive breaking change... And loading the Economy cog to get access to the bank commands would also load things such as [p]payday and would require changes by all Cog creators for their cogs that interface with the Bank API, although these breaking changes could be kept to a minimum (just renaming a few variable/function and renaming the import module)

In the long term I do plan to add some extra features to it though such as bringing a variant of @mikeshardmind's Economy Trickle into it to allow for passive currency gain, and open to other users suggestion.

[mikeshardmind]

and rename the redbot.core.bank to redbot.core.economy

Firm no from me on moving the bank API. Discussing rearranging the cogs is fine, but moving an API just to rename it isn't on the table right now.

Another benefit of this in my eyes is that it gives the sense of more "complete" cogs for cogs that are part of core, aside from bank all other core cogs are full of features, and the state of bank and economy always been odd as for economy bank is always needed if the user wants to setup basic balances, currency name which are part of the wider bot economy.

As I said on discord, the point of the bank cog isn't to be a complete user facing feature filled cog. Much like modlog, it exists to provide a framework for 3rd party cogs to use cooperatively. It being sparse isn't a downside, it leaves room for people to customize while getting the cooperative behavior.

For the record, another strong objection, stronger than mine on this particular route

In theory, I'm in favor of not having users need to load a separate cog to manage bank settings, but I'm not 100% convinced the answer is to merge it with Economy. With more thought on this since discussed in discord though, I think the right option here isn't a merge of cogs, but to move management of (both) the bank (and modlog) API(s) to core_commands.py as these are fundamental frameworks for Red, whereas the other commands in Economy are more things which use that framework to provide basic uses of it.

[Jackenmen]

I'm not fond of the idea of moving this stuff to core commands, yes, they are pretty much basic interfaces for bank and modlog API, but it's one thing to just have an api that gives you something that you can build around and other to have that interface built in to core commands. Red is supposed to be modular and by adding this stuff into core commands instead of having them in separate cogs, we limit that modularity. I might not want to use bank at all and all I gotta do now is just unload the cog, my commands namespace is clean, I can easily use those commands names for something else, just having unused API doesn't affect me in any meaningful way. If we put them in core commands however, the best I can do is disabling all the bank commands (which is bothersome to say the least) and my commands namespace is polluted with the commands I simply don't want.

For the other case - modlog, one could say that they really like Modlog API but don't like how modlog cog interface uses it. So they decide to create their own interface for it, either by subclassing modlog cog if they only have problem with let's say reason command being limited to the moderator that created the case or they create a whole new interface for modlog api in their own cog with changed UI, maybe some additional integration with some kind of warn system they created, I don't know, the thing is possibilities are countless and I think it's bad thing to limit them in such way.

What I wanted to say is, in my opinion negatives outweight the positives of this change (what really are the positives?)

Edit: Looks like I completely forgot about the commands for settings ([p]modlogset and [p]bankset) which make sense to be put in core commands as they edit modlog and bank api settings. Sorry about that. My point still stands for the rest of the commands - I wouldn't want [p]case and alike to be in core commands.

[Jackenmen]

Forgot about one thing I think is worth changing - commands under [p]bank being in Economy cog. I don't think it's very intuitive and I think it would actually make more sense to have them in Bank cog.

[Drapersniper]

Forgot about one thing I think is worth changing - commands under [p]bank being in Economy cog. I don't think it's very intuitive and I think it would actually make more sense to have them in Bank cog.

Yeah That was the sole reason behind #2961

[mikeshardmind]

@jack1142 Regarding polluted command namespaces: if we moved them to core_commands, I'd assume we would migrate them to be subcommands of set to avoid that exact problem.

If this wasn't a suitable way to handle it, keeping in mind that the API commands (modlogset, bankset) stay in effect even without their associated cogs loaded since they are merely a user front end for the APIs, we could have a cog dedicated to managing these API specific settings (shared between all cogs present and future which do this) which could be loaded only when needed by the owner to change settings.

[Flame442]

I have to agree with Sinbad here. The bank cog currently only contains the [p]bankset commands. These commands won't be seen by the average user anyway, so putting them in core_commands would not cause an issue in help (except for those who can use it, however putting it in [p]set would solve that as well). It would make economy be the cog users can load if they actually want to interact with the bank (since it contains the balance commands and the slot/payday commands), and users can keep unloaded if they do not want to use the bank without any bank-related commands still being shown to the users.

+1 from me for [p]bankset -> core_commands.

[Jackenmen]

Okay so I somehow missed the whole point of this issue...

Gotta take a reading course 🤦‍♂

+1 from me for [p]bankset and [p]modlogset -> core_commands as well

For the whole time I thought this is about having the commands like [p]bank, [p]casesfor, [p]case, [p]reason being moved to core_commands which is why I wrote the whole comment, ugh... If this means that those commands will still stay in ModLog cog (and Bank cog? - read below) and the only commands we're talking about here are [p]bankset and [p]modlogset then yeah, you guys are right it makes sense to move those.

Using the occasion though, I would still like to propose keeping Bank cog - it wouldn't have [p]bankset but it could still have [p]bank moved to it. I guess it isn't that important and we could just remove it fully but I'm leaving that up for discussion.

Now gotta take that reading course...

[Drapersniper]

I'd personally be in favor for Keeping the [p]bank command into Economy, and deprecating the Bank cog, moving [p]bankset to core_commands.py.

Loading 1 Cog to have 1 Command group feels, lack luster and it kinda goes against the "no cogs with just 1 command" approval criteria we set for Cog creators

[mikeshardmind]

Loading 1 Cog to have 1 Command group feels, lack luster and it kinda goes against the "no cogs with just 1 command" approval criteria we set for Cog creators

Again, just because it has one command, doesn't mean that command is all that the bank (as a whole, not just the cog) does. Saying it only has one command and shouldn't be a cog because of that is extremely disingenuous and really needs to not be part of the discussion about this.

While the front end only provides a single (top level) command (currently), the bank provides a centralized way for cogs to interact with a credit system.

For more context, @tekulvw already said this in discord about it

it's not part of the bot code therefore it's not core it's bank they're at the same level in the code, but they aren't the same code but my point is, the logic of why it would get put into core commands is flawed.

I personally (partially) disagree with this portion. The cog is a seperate cog, but the cog only manages settings to a public API which is not replaceable by end users (located at redbot.core.bank)

I do agree that core settings is already rather large though, and moving these commands here expands this. There may be an alternative way to handle this to explore later but for now...

but short story, don't put bankset in core commands. Move account management into bank

(again quoting Will) is something I can agree on for current handling.

[palmtree5]

The cog is a seperate cog, but the cog only manages settings to a public API which is not replaceable by end users (located at redbot.core.bank)

Exactly this. That was the purpose of the Bank cog. The reason it's separate from Economy was so if someone were to replace the Economy cog with their own substitute Economy cog, they would still be able to use the bank API with their substitute cog without having to implement their own commands to manage the bank as those commands are in a separate cog.

I am 100% against merging Bank back into Economy. That split was done for a reason

[mikeshardmind]

Discussion of why the split happened already covered and not likely to change.

For the sake of user experience, we can look at making the commands show up grouped anyway later on, but there's not more to discuss on actually merging the cogs.