idoneam / Canary

Canary is a Python3 bot designed for the McGill University Community Discord Server. The bot provides helper functions to users, as well as fun functions, a quote database and custom greeting messages.
GNU General Public License v3.0
11 stars 7 forks source link

[Enhancement] Generalize bot #108

Closed le-potate closed 5 years ago

le-potate commented 5 years ago

Since we tell people how to use the bot on their server, a few things should be different -Find all the direct mentions of server/channel IDs, upmartlet/downmartlet, roles (idoneam and Discord Moderator are the only roles referred to afaik), and take those values from config instead -Maybe do the same thing for all mentions of "marty" (except variables, name of the db, etc), take it from a botName in the config file instead -In main.py, take "command_prefix" from config -Add something to the readme to say to change those values in the config file There are not that many files with things like that, so far I've found: main.py, mod.py, reminder.py, score.py but if you see anything else please tell me in the comments

le-potate commented 5 years ago

I think the only one of those I won't be able to do is command_prefix because it needs to be defined before the parser is called. I'll just add a mention in readme

trungams commented 5 years ago

Define a class Canary (or however you want to call it) that inherits discord.py's bot object. Do all the parsing inside __init__(), then you can do whatever you want

trungams commented 5 years ago

that should be a better way to implement a custom discord bot, we're just lazy

le-potate commented 5 years ago

Right now it only calls parser.Parser() if you run the file, not if you import it, but if I do that it will parse everytime right? Is that okay?

idodin commented 5 years ago

This should be broken down into smaller tasks though - in order to better keep track on who is doing what :p

And yeah, when I wrote the Parser, I knew that I should probably override init, I was just lazy and excited to get done with config hehe.

idodin commented 5 years ago

Whoops, I actually misread "Participants" for "Assignees" @le-potate - its fine if you want to tackle this on your own; but break down into smaller Issues in any case so that we can see what's going on more easily in commits :) .

idodin commented 5 years ago

Right now it only calls parser.Parser() if you run the file, not if you import it, but if I do that it will parse everytime right? Is that okay?

parser.Parser() is a Constructor. If you use it inside the __init__() function of a new "specific Bot" Class, it will only be run whenever you construct the Bot object - which is the desired behaviour :o

le-potate commented 5 years ago

This should be broken down into smaller tasks though - in order to better keep track on who is doing what :p Whoops, I actually misread "Participants" for "Assignees" @le-potate - its fine if you want to tackle this on your own; but break down into smaller Issues in any case so that we can see what's going on more easily in commits :) .

No I agree, I didn't think about the stuff in main.py that happens before it's parsed so I thought this would be something I could do alone. I'm nearly finished with the other files, so i'm gonna commit that and then you guys can figure out main.py

le-potate commented 5 years ago

@idodin @trungams Branch issue-108 now contains the modified config.ini, parser.py, mod.py, reminder.py and score.py files. There's only Main.py left

trungams commented 5 years ago

Put some code in ee94b57f4f8d3cec35501039491e9a6f375793e6. Someone pls test

trungams commented 5 years ago

https://www.youtube.com/watch?v=dQw4w9WgXcQ

le-potate commented 5 years ago

@trungams Not related to this issue but to your commit, do you think all print statements should be logged instead? If so, I found one in main.py that wasn't changed and one in currency.py

trungams commented 5 years ago

@le-potate it's pretty much relevant, since we're trying to make the modules/cogs work together, it would be great if all of them write to the same log.

and yeah, I just put the main logger object in the bot yesterday, wasn't motivated enough to go figure out the rest without being able to test anything. I wasn't sure if it works at all. If anyone wants to take on logger i'd appreciate it. Note that there are different levels of logging, each with their own function call to log

https://docs.python.org/3.6/library/logging.html#logging.Logger

le-potate commented 5 years ago

Do you know if there's a way that every exceptions could be logged, i.e without having to add a line everywhere where there could be errors in each file

trungams commented 5 years ago

there should be a master function that is responsible of queuing up and dispatching the commands issued by users. look for that function and override it: try calling the command's function, log errors if there's any, and raise the same exception again so that the command's function will have to handle it

le-potate commented 5 years ago

Since writing this I found a way that might be simpler than that by re-assigning sys.excepthook (See this and that) What do you think?

trungams commented 5 years ago

I think discord.py is already doing similar things. You can see marty still runs if an error is thrown uncaught in any command. The bot would exit immediately if we're using the default exception handler. So make use of what the library already has, don't bother with sys

le-potate commented 5 years ago

Just tested the latest issue-108 branch a bit, for me canary.log gets created but nothing gets written in it, including the ?mix logs.

le-potate commented 5 years ago

issue-108* oops

trungams commented 5 years ago

absolutely nothing or were there some warning information?

le-potate commented 5 years ago

nothing

trungams commented 5 years ago

in config/config.ini, change log level to info or debug, it's gonna be more verbose

le-potate commented 5 years ago

yep that's it, both work (warning didn't print anything)

le-potate commented 5 years ago

debug prints too much stuff, I think info is more useful

trungams commented 5 years ago

yeah, debug is just for development purpose

le-potate commented 5 years ago

we could also change the bot.logger.info to something like bot.logger.warning if we don't want the stuff that comes with info either, only what we log

le-potate commented 5 years ago

@trungams can you look at f32a07a0256392ccd515ececbb782fe001804f39, seems to works quite well for me

le-potate commented 5 years ago

it's exactly the on_command_error function from discord.py but with the two logger lines added so everything should still work

le-potate commented 5 years ago

image 😄

trungams commented 5 years ago

Looks good. Can you put them in bot.py without the @bot.event decorator? I want to keep main.py minimal.

It's gonna be async def on_command_error(self, ctx, error): inside Canary class

le-potate commented 5 years ago

yup

le-potate commented 5 years ago

@trungams done in 2862937318c03aeaf3860139307b996cc414c810

trungams commented 5 years ago

cool

le-potate commented 5 years ago

In mod.py, I want to change @commands.has_role('Discord Moderator') to @commands.has_role(self.bot.config.moderator_role) but self and bot aren't defined there since it's outside the functions, what would be the best thing to do

trungams commented 5 years ago

use a global variable and set it inside init, can't think of a better way

le-potate commented 5 years ago

that doesn't seem to work since it's inside the class but outside any method

trungams commented 5 years ago

image

le-potate commented 5 years ago

Yup that's exactly what I did, with the None at the top and everything, but it doesn't work, when you try to pm you get a check failure. I put a print statement just before line 44 to check, and moderator_role is still none at that point

le-potate commented 5 years ago

Oh and btw for your line 30 you have a syntax error because you can't put the equal on the same line (like it would have to be global mod_command mod_command = self.bot.config.moderator_role )

trungams commented 5 years ago

dunno

le-potate commented 5 years ago

I'm gonna try to find other ways tomorrow, but I can always just rewrite it so that the check gets done inside the method instead if we don't find anything

trungams commented 5 years ago

just hardcode it lol

trungams commented 5 years ago

check my 2 recent commits on how to customize checks. for reference: https://discordpy.readthedocs.io/en/latest/ext/commands/api.html?highlight=missingrole#discord.ext.commands.check

also this conversation should be moved to a pull request now. we have basically everything needed