Skip to content

Conversation

WizardCM
Copy link
Member

image

After a few minutes of using the bot, I realised it'd benefit from some basic colour coding for logging levels.

@derrod
Copy link
Member

derrod commented May 10, 2021

Few things:

  1. For consistency with the rest of the code, use single-quoted strings
  2. printf style string formatting should not be used, use the modern formatting methods (.format/f-strings) instead
  3. There really isn't any reason for formatting at all, the level names don't change, so you can just write out the level name + console colour codes for each, also saves the unnecessary string concatenation with the RESET part
  4. The way this is currently done would affect both the console and file log handlers, the logfile should not contain terminal control characters and remain plain text, you may have to introduce a separate log formatter for console logging

@WizardCM
Copy link
Member Author

  1. did cross my mind as I went to bed. Definitely worth fixing.
    I'll get all 4 points fixed this week.

@WizardCM
Copy link
Member Author

Alright, I removed formatting entirely & ensured it doesn't affect log files. Let me know if it needs anything else.

@derrod
Copy link
Member

derrod commented May 17, 2021

Well this is a bit of an issue.
screenshot
I wonder if there's a cross-platform way to do this.

@WizardCM
Copy link
Member Author

Hmm, this is a good point. I don't feel particularly comfortable importing a dependency just to colour logs. I'll do some further digging. Do we want to support cmd on Windows 10 as the minimum?

@derrod
Copy link
Member

derrod commented May 17, 2021

Yeah, somewhat selfishly since I do most of my work on windows, so it should work there. Haven't tested in powershell, but no sane person uses powershell.

@tt2468
Copy link
Member

tt2468 commented May 17, 2021

I don't feel particularly comfortable importing a dependency just to colour logs.

The bot is already using third party requirements (requirements.txt), so I don't see why it would be a bad thing. I personally believe that it's better to depend a package that results in more readable code than it is to try to build in that functionality myself in situations like this.

@WizardCM WizardCM marked this pull request as draft July 3, 2021 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants