Adds logfile and loglevel optional parameters #63
Conversation
There was a problem hiding this comment.
This is mergable as is. I added a couple comments that could be improvements, though. Should probably give a nice error message when the log level is invalid too. I also, personally, prefer --log-level and --log-file to --loglevel and --logfile, but perhaps that is just opinion.
| loglevel = logging.DEBUG | ||
| loglevel_str = "DEBUG" | ||
| loglevels = { "CRITICAL": logging.CRITICAL, "ERROR": logging.ERROR, "WARNING": logging.WARNING, "INFO": logging.INFO, | ||
| "DEBUG": logging.DEBUG, "NOTSET": logging.NOTSET } |
There was a problem hiding this comment.
Might be easier on the eyes to have these on separate lines rather than two longish ones
| loglevel_str = args.loglevel | ||
| elif "loglevel" in bot.config: | ||
| loglevel = loglevels[bot.config["loglevel"]] | ||
| loglevel_str = bot.config["loglevel"] |
There was a problem hiding this comment.
Could just put loglevel = loglevels[loglevel_str] after the if/else block. Would also allow you to chop off line 69. Would then have a common place to check for a bad log level too.
There was a problem hiding this comment.
I agree with this, loglevel assignment can be factored out to make it a bit clearer
| bot = halibot.Halibot() | ||
| bot._load_config() |
There was a problem hiding this comment.
Not an issue, but the config will be loaded twice when running a bot.
I think this is more arguments that we need some kind of solution to #57
|
|
||
| # Error check log level string | ||
| if loglevel_str not in loglevels: | ||
| print("Log level {} invalid. Continuing with log level {}".format(loglevel_str, loglevel)) |
There was a problem hiding this comment.
This will print ...Continuing with log level 10 I think. I have no issue with this, but it might just be worth defaulting to saying "DEBUG" there too
There was a problem hiding this comment.
Or should this be a runtime error? Passing an invalid log should maybe just be irrecoverable?
There was a problem hiding this comment.
Whoops, you're right. And it doesn't matter to me, should it just throw a runtime error in that case?
There was a problem hiding this comment.
@sjrct Thoughts on this? I'm starting to think that this should be an error, since it implies misconfiguration. Rather than have it limp along, should it just give up and yell at you?
There was a problem hiding this comment.
Yeah, this should be a runtime error. Should fail like any other incorrectly specified argument.
|
Huh, the build broke on 3.5 for some reason, but I can't see anything related to it in this commit; looks like a fluke to me. Unless @sjrct has any complaints about that, I might just merge it anyway. |
|
I have no complaints |
|
We should certainly fix that problem though... |
Addresses #46, please review.
Note: logging defaults to stdout, DEBUG level. Parameters can be specified via command line or in config.json file.