Custom Logger class

Question

My custom Logger class works just fine. However, the class is not intended for making instances of. Because there is really no reason to make instances of my class, or so I think.

Any constructive criticism? Is this considered good "design"?

I have a feeling that I shouldn't open and close my file for each method call, is that bad? Is the way I open/write/close files in my Logger class bad?

from Time import Time
from datetime import datetime
temp = datetime.now().today()
YEAR, MONTH, DAY = temp.year, temp.month, temp.day
class Logger:
 """If a log file for today already exist, open it in append mode.
 Else, create a new log file for today, and open it in append mode.
 """
 DEBUG = False
 PADDING = 9 # right-padding for type names in the log
 filename = f"logs/log{DAY}-{MONTH}-{YEAR}.txt"
 file = open(filename, "a").close()
 """Clears the log file"""
 @staticmethod
 def clear():
 with open(Logger.filename, "r+") as file:
 file.truncate(0)
 """Log info"""
 @staticmethod
 def log(msg):
 with open(Logger.filename, "a") as file:
 type = "INFO".ljust(Logger.PADDING)
 file.write(f"|{type}|{Time.getHour()}:{Time.getMinute()}| {msg}\n")
 "Used to log whenever a state is updated"
 @staticmethod
 def update(msg):
 with open(Logger.filename, "a") as file:
 type = "UPDATE".ljust(Logger.PADDING)
 file.write(f"|{type}|{Time.getHour()}:{Time.getMinute()}| {msg}\n")
 "Only logs if the static variable {DEBUG} is set to True."
 @staticmethod
 def debug(msg):
 if not Logger.DEBUG: return # Only log if DEBUG is set to True
 with open(Logger.filename, "a") as file:
 type = "DEBUG".ljust(Logger.PADDING)
 file.write(f"|{type}|{Time.getHour()}:{Time.getMinute()}| {msg}\n")
 """Intended for use on objects. They usually have a lot of information;
 therefor, we enclose the information with lines to make the log more readable."""
 @staticmethod
 def object(object):
 with open(Logger.filename, "a") as file:
 file.write(f"----------------------- object log\n")
 file.write(object)
 file.write(f"\n-----------------------\n")
 @staticmethod
 def exception(msg):
 with open(Logger.filename, "a") as file:
 type = "EXCEPTION".ljust(Logger.PADDING)
 file.write(f"|{type}|{Time.getHour()}:{Time.getMinute()}| {msg}\n")
if __name__ == "__main__":
 Logger.log("This is a test")
 Logger.log("running something")
 Logger.debug("Some debugging details")
 Logger.exception("Critical error!")
 Logger.debug("Some more debugging details")

Output example:

|INFO |10:35| This is a test
|INFO |10:35| running something
|DEBUG |10:35| Some debugging details
|EXCEPTION|10:35| Critical error!
|DEBUG |10:35| Some more debugging details

• Peilonrayz: Have you intentionally reinvented the logging module?
• Alexander Nielsen: @Peilonrayz No, my goal was not to reinvent the logging module. That said, the point of this class is the same as that of the logging module -- to log. I choose to create my own logging class for my project so that I didn't have to deal with the logging module, and so that I could more easily tailor it to do exactly as I wanted.

Answer 1

• Docstrings should be on the inside of a function or class, not on the outside - before the def or class.
• Your logger doesn't automatically log to the next day's log. As YEAR, MONTH, DAY and filename are only defined once and never update.
• It's abnormal to see Logger.filename rather than self.filename or cls.filename. If you need to guarantee it's the class' value rather than the instances than you can do type(self). This makes it harder to rename your class if you ever need to in the future.
• Your functions should be classmethods as they're interacting with the class. staticmethod is just wrong.
• Due to a combination of the above two your class can't be instanced or subclassed. This is an artificial limitation and is un-Pythonic.
• It's normally better to use import x rather than from x import .... The only time I've found this to not be the case, is when you're importing from typing. But every other time I've found code to be more readable. As you know what the function is, without scrolling to the top of the file.
• There's no benefit to using datetime.now().today() as they return the same object, but with a slightly different time due to the delay in each call.

• It would be better to define the format once. Currently you're duplicating it across all functions. To do so you would need to pass a datetime / time object and you can just use the format specifier.

  f"|{type}|{datetime:%I:%M}| {msg}\n"
  

• You don't need to manually call str.ljust you can just change your format to include:

  f"{type:< {Logger.PADDING}}"
  

• Don't use return on the same line as an if. It's un-Pythonic.

• PEP 8 - Python's style guide - suggests using only 2 spaces before an inline comments #. You've used 1 and 5.
• Personally I dislike inline comments as they're hard to read. Putting the comment before the line has normally always been more readable for me.
• I suggest you rename log to info and allow log handle pretty much everything. For example interaction with the format string and also not logging if it's a debug message.

• You could just open the file once and leave the file object to be closed when Python exits - as that the only time you'd want to actually close the file.

  If you're sceptical that Python will clean up the file on exit you can just register an exit handler with atexit.

• Your log levels aren't great. What if I want to ignore non-exceptions?

• You don't log to std.out or std.err, this is a short coming.

import datetime
class Logger:
 """If a log file for today already exist, open it in append mode.
 Else, create a new log file for today, and open it in append mode.
 """
 DEBUG = False
 PADDING = 9
 FORMAT = "|{type:< {cls.PADDING}}|{datetime:%I:%M}| {msg}\n"
 FILE = open(f"logs/log{datetime.datetime.now():%d-%m-%Y}.txt", "w+")
 @classmethod
 def log(cls, msg, level):
 if not cls.DEBUG and level == "DEBUG":
 return
 cls.FILE.write(cls.FORMAT.format(
 type=level,
 msg=msg,
 datetime=datetime.datetime.now(),
 ))
 @classmethod
 def info(cls, msg):
 """Log info"""
 cls.log(msg, "INFO")
 @classmethod
 def update(cls, msg):
 "Used to log whenever a state is updated"
 cls.log(msg, "UPDATE")
 @classmethod
 def exception(cls, msg):
 cls.log(msg, "EXCEPTION")
 @classmethod
 def debug(cls, msg):
 "Only logs if the static variable {DEBUG} is set to True."
 cls.log(msg, "DEBUG")
 @classmethod
 def clear(cls):
 """Clears the log file"""
 cls.FILE.truncate(0)
 @classmethod
 def object(cls, object):
 """Intended for use on objects. They usually have a lot of information;
 therefor, we enclose the information with lines to make the log more readable."""
 cls.FILE.write(
 f"----------------------- object log\n"
 + str(object)
 + f"\n-----------------------\n"
 )
if __name__ == "__main__":
 Logger.info("This is a test")
 Logger.info("running something")
 Logger.debug("Some debugging details")
 Logger.exception("Critical error!")
 Logger.debug("Some more debugging details")

Logging

But why not just use logging? You don't have to care if you've handled the file correctly, and you can get pretty much everything you want with ease. The entire of the logging module was created to be highly customizable. It's why it's so ridiculously complex.

As an example of defining UPDATE with a level of 5. And printing everything by default. With some functions removed for brevity.

import datetime
import logging
def _update(self, message, *args, **kwargs):
 if self.isEnabledFor(5):
 self._log(5, message, args, **kwargs)
logging.basicConfig(
 filename=f"logs/log{datetime.datetime.now():%d-%m-%Y}.txt",
 format="|%(levelname)-9s|%(asctime)s| %(message)s",
 datefmt="%I:%M",
 level=0,
)
logging.addLevelName(5, "UPDATE")
logging.Logger.update = _update
if __name__ == '__main__':
 logging.debug("a")
 logging.info("a")
 logging.error("a")
 # update and object are not defined on the module
 # However you can do something similar to defining it on the class.
 logger = logging.getLogger('foo_application')
 logger.debug("b")
 logger.info("b")
 logger.error("b")
 logger.update("b")

• Hyperplane: Monkey patching à la logging.Logger.update = _update does not play nice with mypy, unfortunately.
• Peilonrayz: @Hyperplane Yes monkey patching with duck typing won't 'play nice' with static typing. However you can just # type: ignore or make an ILogger Protocol.

Answer 2

Yes, this is not a good class design. You are mixing global state with in your methods and using it only as a namespace. Instead consider something like this:

from Time import Time
from datetime import datetime
class Logger:
 """If a log file for today already exist, open it in append mode.
 Else, create a new log file for today, and open it in append mode.
 """
 DEBUG = False
 PADDING = 9 # right-padding for type names in the log
 def __init__(self, filename=None):
 if filename is None:
 today = datetime.now().today()
 filename = f"logs/log{today.day}-{today.month}-{today.year}.txt"
 open(filename, "a").close() # is this needed?
 def clear(self):
 """Clears the log file"""
 with open(self.filename, "r+") as file:
 file.truncate(0)
 def log(self, msg, level="INFO"):
 """Log at level"""
 with open(self.filename, "a") as file:
 type = level.ljust(self.PADDING)
 file.write(f"|{type}|{Time.getHour()}:{Time.getMinute()}| {msg}\n")
 def info(self, msg):
 """Log info"""
 self.log(msg, "INFO")
 def update(self, msg):
 """Used to log whenever a state is updated"""
 self.log(msg, "UPDATE")
 def debug(self, msg):
 """Only logs if the static variable {DEBUG} is set to True."""
 # Only log if DEBUG is set to True
 if not self.DEBUG:
 return
 self.log(msg, "DEBUG")
 def object(self, obj):
 """Intended for use on objects. They usually have a lot of information;
 therefor, we enclose the information with lines to make the log more readable."""
 with open(Logger.filename, "a") as file:
 file.write(f"----------------------- object log\n")
 file.write(obj)
 file.write(f"\n-----------------------\n")
 def exception(self, msg):
 self.log(msg, "EXCEPTION")
if __name__ == "__main__":
 logger = Logger()
 logger.log("This is a test")
 logger.log("running something")
 logger.debug("Some debugging details")
 logger.exception("Critical error!")
Logger.debug("Some more debugging details")

Note the docstring conventions which require the string to be inside the methods and not shadowing the built-in object.

• Sebastian Nielsen: But if I don't HAVE to created instances of the Logger class, why should I? (as you did)
• Graipher: @SebastianNielsen: In that case you don't need a class at all, they can just be stand-alone functions in a separate module, since you are using the class only as a namespace (with the overhead of having to declare every method static. So instead of from logging import Logger; Logger.log("...") you do import logging; logging.log("...").
• Graipher: @SebastianNielsen: Using your code also means that you can never have two loggers in two separate processes, they will always all write to the same file.
• Graipher: @SebastianNielsen: In other words, what's the point of a class if you never instantiate it or inherit from it?

Answer 3

Just to add to the other answers, not sure if you've considered this, but keep in mind that the file name will be the same unless you create a new instance of your class.

That means that if your logger instance is the same across midnight, you'll have the log of things that happened in one day written in the file of the previous day.

Maybe this is actually what you want? Again, not sure. But consider instead something along the lines of:

def get_log_filename():
 today = datetime.now().strftime(format="%d-%m-%Y")
 return f"logs/log{today}"
class Logger:
 def log(self, msg):
 with open(get_log_filename(), "a") as file:
 log_type = "INFO".ljust(Logger.PADDING)
 file.write("|{log_type}|{Time.getHour()}:{Time.getMinute()}| {msg}\n")

Another couple of things that I wanted to mention:

• I understand wanting to put docstrings, but they should either be meaningful or not be there at all, otherwise it's just polluting the code. Having a method called clear with a docstring that says clears the logfile isn't really useful.

• I'd also avoid naming a variable type, because not only it's very generic, there's also the built-in function type. The common practice when name clashes happen is to add an underscore at the end (type_), but in this case I think log_type is much better