Python Output Class

Question

I have the following Python output class that I'm planning to use across a number of modules in a terminal application:

from colorclass import Color
from colorclass import disable_all_colors, enable_all_colors, is_enabled
from time import gmtime, strftime
class OutputHelper(object):
 def __init__(self, color, domain):
 if color:
 disable_all_colors()
 self.domain = domain
 def Terminal(self, severity, message):
 leader = ''
 if severity == 1:
 leader = Color('{green}[GOOD]{/green}')
 elif severity == 2:
 leader = Color('{cyan}[INFO]{/cyan}')
 elif severity == 3:
 leader = Color('{yellow}[LOW]{/yellow}')
 elif severity == 4:
 leader = Color('{magenta}[MEDIUM]{/magenta}')
 elif severity == 5:
 leader = Color('{red}[HIGH]{/red}')
 elif severity == 6:
 leader = Color('{red}[!!CRITICAL!!]{/red}')
 else:
 leader = '[#]'
 print('[{}] [{}] {} {}'.format(strftime("%H:%M:%S", gmtime()), self.domain, leader, message))

The idea being that one module can raise multiple messages for a single domain. So usage would be similar to:

output = OutputHelper(arguments.nocolor, arguments.domains)
output.Terminal(1, 'All dependencies up to date')
output.Terminal(6, "Cryptojacking script identified: aaaa")

I'm sure there's better ways to design this, but I don't know what I don't know, and I'd value input and guidance!

Answer 1

if - elif

instead of the if - elif statements, use dict.get(key, default):

formatting = {
 1: Color('{green}[GOOD]{/green}'),
 2: Color('{cyan}[INFO]{/cyan}'),
 3: Color('{yellow}[LOW]{/yellow}'),
 4: Color('{magenta}[MEDIUM]{/magenta}'),
 5: Color('{red}[HIGH]{/red}'),
 6: Color('{red}[!!CRITICAL!!]{/red}'),
}
leader = formatting.get(severity, '[#]')

String formatting

if you use Python>= 3.6, use f-strings.

And instead of the long list of arguments, split it up in lines. I would at least refactor the strftime("%H:%M:%S", gmtime()) into a variable called time:

message = f'[{time}] [{self.domain}] {leader} {message}'
print(message)

if you also have to serve pre-3.6, this solution:

format_args = {
 'time': strftime("%H:%M:%S", gmtime()),
 'domain': self.domain,
 'leader': leader,
 'message': message,
}
template = '[{time}] [{domain}] {leader} {message}'
print(template.format(**format_args))

can help you limit the line length, while keeping the template clear.

• Kevin: @Daniel: try/except is for when you want to run some genuinely different code in the exceptional case. Here, we just want to get a different value. If your suggestion were accurate, there would be no valid use case for dict.get() and the language would not have included it.
• Graipher: In the str.format alternative, it should be [{domain}] instead of [{self.domain}].

Answer 2

1. You should make effort to follow the guidelines listed in PEP-8, the official style guide for Python.

   • The imports should be separated into three distinct categories, the first being standard library imports, then related third-party imports, and finally local project imports. The categories are separated by a single blank line. I'm assuming colorclass is colorclass on PyPi, current version 2.2.0. If that's the case, the imports should be:

     from time import gmtime, strftime
     from colorclass import Color
     from colorclass import disable_all_colors, enable_all_colors, is_enabled
     

   • Each top-level function or class definition should be preceded by two blank lines. This is done to make each definition visually distinctive.

   • OutputHelper, being the name of a class, correctly follows the PascalCase naming convention, but OutputHelper.Terminal() should be OutputHelper.terminal(), as functions and method names should follow the snake_case naming convention.

2. In Python 3, all classes are new style classes, so the explicit object superclass is completely unnecessary.

3. The name OutputHelper.Terminal() does not adequately summarize what the method does. To me, that sounds almost like a 'terminal factory' method that spits out a unique Terminal instance each time it's called. Since what it really does is log something, why not call it OutputHelper.log()?

4. In a similar vein, why is the class called OutputHelper? Of course, it interacts with the output stream, and of course, it's a 'utility' in a way, but Logger is far more accurate.

5. Stdout is not for debugging. You should write these kinds of messages to stderr instead (sys.stderr).

6. The numbers one through six don't have any significance, they're pretty much arbitrary values for log severity. It makes sense to give them a unique name, which not only makes them easier to remember, but also reduces the chance of typos.

   from enum import IntEnum
   # ...
   class LogLevel(IntEnum):
    GOOD = 1
    INFO = 2
    LOW = 3
    MEDIUM = 4
    HIGH = 5
    CRITICAL = 6
   

   to be used as:

   Logger.log(LogLevel.GOOD, "All dependencies up to date.")
   

7. Talking about the output itself, that could use some work too. 'LOW / MEDIUM / HIGH'? What, exactly? Probably severity, so I suggest you change it to 'LOW / MEDIUM / HIGH SEVERITY ERROR' instead.

8. In OutputHelper.__init__():

   if color:
    disable_all_colors()
   

   If color is a boolean flag*, shouldn't this be a negation? Surely you'd only disable the colors if colors are not supported, not the other way around.

9. leader doesn't have to be initialized as an empty string, since you already deal with the else: case.

10. There's something funny about the current concept of domains. You say each module could log for a single domain, which suggests you instantiate multiple loggers and point them at one domain. That's fine, but you could achieve the same with a simple function, and just have the domain be an argument. Is a module tied strictly to one instance?*

    Even if you need the class, arguments.domains (plural, in the second snippet), suggests one instance logs to multiple domains. Though the code you showed us doesn't confirm that, it may hold true for the complete program. Or it may just be a typo. Either way, you should look into that.

11. I'm sure you're aware there's the standard library logging module.

I end up with something like this:

from enum import IntEnum
from sys import stderr
from time import gmtime, strftime
from colorclass import Color
from colorclass import disable_all_colors, enable_all_colors, is_enabled
class LogLevel(IntEnum):
 GOOD = 1
 INFO = 2
 LOW_SEVERITY_ERROR = 3
 MEDIUM_SEVERITY_ERROR = 4
 HIGH_SEVERITY_ERROR = 5
 CRITICAL_ERROR = 6
class Logger:
 def __init__(self, color_supported, domain):
 if not color_supported:
 disable_all_colors()
 self.domain = domain
 def log(self, level, message):
 if level == LogLevel.GOOD:
 leader = Color('{green}[GOOD]{/green}')
 elif level == LogLevel.INFO:
 leader = Color('{cyan}[INFO]{/cyan}')
 elif level == LogLevel.LOW_SEVERITY_ERROR:
 leader = Color('{yellow}[LOW]{/yellow}')
 elif level == LogLevel.MEDIUM_SEVERITY_ERROR:
 leader = Color('{magenta}[MEDIUM]{/magenta}')
 elif level == LogLevel.HIGH_SEVERITY_ERROR:
 leader = Color('{red}[HIGH]{/red}')
 elif level == LogLevel.CRITICAL_ERROR:
 leader = Color('{red}[!!CRITICAL!!]{/red}')
 else:
 leader = '[#]'
 print('[{}] [{}] {} {}'.format(
 strftime("%H:%M:%S", gmtime()),
 self.domain,
 leader,
 message
 ),
 file=stderr
 )

---

* Bonus points: Add a docstring. Every public function should include a summary in the form of a docstring (triple quoted multiline string literal, but that's a mouthful), which lists the various arguments the function accepts, what it does and what it returns. Both type and value restraints should be mentioned in the docstring.