3
\$\begingroup\$

I have written a small function that will be imported in each working file of my project and will make logging uniform for the project, allowing for centralized editing of logging settings.

What can I improve in my code to make it more reliable and flexible?

logger.py:

import logging


def logger(name = __name__, level: int | str = logging.INFO, filename: str | None = None) -> logging.Logger:
    if isinstance(level, str):
        if level not in ["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"]:
            raise ValueError("Некорректный уровень ло��ирования")

        level = logging.getLevelName(level.upper())

    elif isinstance(level, int):
        if level not in [10, 20, 30, 40, 50]:
            raise ValueError("Некорректный уровень логирования")

    else:
        raise TypeError(f"Неккоректный тип данных уровня логирования")

    if not logging.getLogger(name).handlers:
        format_log = "%(asctime)s | %(levelname)s | %(name)s:%(module)s:%(lineno)s - %(message)s"
        logging.basicConfig(level=level, format=format_log, filename=filename)

    return logging.getLogger(name)


if __name__ == "__main__":
    logger().info("Логер успешно запущен")
\$\endgroup\$

4 Answers 4

5
\$\begingroup\$

Two thoughts.

The error handling is not useful. The routine is used by the programmer, this is not something that depends on user input and needs strong validation somehow. So it probably won't trigger anyway, unless you make a typo somewhere. You can just let Python crash and it will display an error. Your script does not add anything useful.

It is preferable to have a config file of some sort, rather than hardcode the desired level etc. You can for example use a .ini file with the configparser lib.

Then some code like this:

CONFIG_FILE_NAME = "config.ini"


def main():
    logger = logging.getLogger("app")
    try:
        # get current application dir
        current_dir = os.path.dirname(os.path.realpath(__file__))
        ini_file_path = Path(current_dir) / CONFIG_FILE_NAME

        # load configuration settings from file
        config = configparser.ConfigParser()
        config.read(ini_file_path)

        # set logging from config file
        logging.config.fileConfig(ini_file_path)
        logger.info("Application starting")

The .ini file can be along these lines:

[loggers]
keys=root, app

[handlers]
keys=console, file

[formatters]
keys=standard

[logger_root]
level=DEBUG
handlers=console

[logger_app]
qualname=app
level=DEBUG
handlers=console,file
propagate=0

[handler_console]
class=StreamHandler
level=DEBUG
formatter=standard
args=(sys.stdout,)

[handler_file]
class=handlers.TimedRotatingFileHandler
level=DEBUG
formatter=standard
args=('transfers.log', 'd', 5)

[formatter_standard]
format=%(asctime)s - %(filename)s:%(lineno)s - %(funcName)s - %(levelname)s - %(message)s

This is a copy-paste from here I think. It can probably be simplified. Please consider this as an example and not a finished product.

I have used YAML files too. This is a question of preference.

Sometimes, you will want to run the application in different scenarios. For example, in debugging mode you will want to enable logs at level DEBUG, and have a copy of all log messages to a file. In standard mode, it would be sufficient to output to console only, at level INFO or even WARNING to reduce noise. Using a config file makes that a bit easier I think.

Besides, an application of moderate complexity will usually already have a config file of some sort.

Another point is that sometimes we will want to tweak the formatter too. It would be nice to be able to do that without touching the code. As it stands, your routine lacks flexibility. Note that you can output log messages to multiple destinations (eg. console, file) at different levels, but the formatter can be different for destination as well.

There are quite a few available parameters, so using an external config file provides a lot more flexibility.

\$\endgroup\$
4
\$\begingroup\$

Instead of hand-coding the possible logging levels and their numeric values, we should use logging.getLevelNamesMapping(). That also includes any user-defined levels that have been added using logging.addLevelName().

There's no good reason to use level.upper() immediately after we have determined it's a member of LEVEL_STRINGS which are all upper-case. In fact, we should definitely not do that if we want to support user-defined levels.

It's inefficient to call logging.getLogger(name) twice - the result should be the same, so just re-use the found logger.

It seems that the filename argument (possibly others) is effective only on the first call to logger(). E.g. both these messages end up on the standard error stream:

    logger().info("message to stderr")
    logger(filename="/dev/null").warning("message to file")
\$\endgroup\$
3
\$\begingroup\$

Documentation

The PEP 8 style guide recommends adding docstrings for functions. You could describe what you are trying to accomplish with the code and provide an example of how a user would call the function.

Tools

You could run code development tools to automatically find some style issues with your code.

ruff shows this suggestion:

= help: Remove extraneous `f` prefix

on this line:

        raise TypeError(f"Неккоректный тип данных уровня логирования")

Exception

Since your exception messages are not in English, I can not comment on how useful they are. Hopefully, they provide sufficient information for the user to take specific action. For example, if the level is an int, the message would instruct the user to choose one of 10-50 by 10.

\$\endgroup\$
2
\$\begingroup\$

I might:

  • Use match rather than conditionals testing with isinstance.
  • Put the list of levels into a module constant
  • Factor out the common error message as a variable.
  • Break the very long line.
  • Import Optional from typing to replace hints like str | None.
import logging
from typing import Optional

LEVEL_STRINGS = ["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"]
LEVEL_INTS = [10, 20, 30, 40, 50]

def logger(
    name = __name__, 
    level: int | str = logging.INFO, 
    filename: Optional[str] = None
) -> logging.Logger:
    value_err_msg = "Некорректный уровень логирования"

    match level:
        case str():
            if level not in LEVEL_STRINGS:
                raise ValueError(value_err_msg)

            level = logging.getLevelName(level.upper())
        case int():
            if level not in LEVEL_INTS:
                raise ValueError(value_err_msg)
        case _:
            raise TypeError(f"Неккоректный тип данных уровня логирования")

    if not logging.getLogger(name).handlers:
        format_log = "%(asctime)s | %(levelname)s | %(name)s:%(module)s:%(lineno)s - %(message)s"
        logging.basicConfig(level=level, format=format_log, filename=filename)

    return logging.getLogger(name)

if __name__ == "__main__":
    logger().info("Логер успешно запущен")

You should also incorporate docstrings for your function and for the module, noting among other things, accepted values for level.

When level is a string, you've already ensured it must be within a set of all uppercase words, so why are you then calling level.upper()? Doesn't this seem redundant?

\$\endgroup\$
3
  • \$\begingroup\$ It's probably a matter of taste, but I find the OP's if-elif-else chain more clear, and prefer match in more complex scenarios. \$\endgroup\$ Commented yesterday
  • \$\begingroup\$ I considered having the conditionals as guards on the patterns, but I felt like that involved too much duplication. \$\endgroup\$ Commented yesterday
  • \$\begingroup\$ I probably wouldn't use Optional unless this needs to be backwards-compatible, per the best practices. I would still probably use Union[..., None] for clarity even in backwards-compatible code. \$\endgroup\$ Commented yesterday

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.