openSUSE / rng2doc

Converts a RELAX NG schema into documentation
MIT License
8 stars 6 forks source link

Improve Logging #45

Open tomschr opened 6 years ago

tomschr commented 6 years ago

Problem

In most modules there is this line:

import logging
# ...
LOG = logging.getLogger(__name__)

However, we have already a logging module: rng2doc/log.py.

Solution

Use the "right" logging module. :wink:

jloehel commented 3 years ago

Hi Tom,

can you please explain this. Usually I use module-level logger like it is described here: https://docs.python.org/3/howto/logging.html#advanced-logging-tutorial. What logger should I use instead? Should I use a single package logger?

tomschr commented 3 years ago

hi Jürgen, :wave:

well, I guess you can initialize the logger instance in rng2doc/log.py. Under Configuring the logger you can see an example. To adapt that to our code, I could imagine something like this:

# log.py

# create logger
log = logging.getLogger("rng2doc")
log.setLevel(logging.DEBUG)  # Not sure if this is needed

# create console handler and set level to debug
ch = logging.StreamHandler()
ch.setLevel(logging.DEBUG)  # This should be set in cli.py

# create formatter
formatter = CustomConsoleFormatter('')

# add formatter to ch
ch.setFormatter(formatter)

# add ch to logger
log.addHandler(ch)

Whenever you need this logger, import it as from log import log and use it like log.debug("Hi").

I haven't tested it, but that would be the idea. If we don't need the CustomConsoleFormatter class anymore, the whole setup would be a bit simpler.

jloehel commented 3 years ago

Thanks for the explanation. I thought logging.getLogger("rng2doc")/logging.getLogger(__package__) will return always the same object. What advantage do we get importing it from log.py? Draft: https://github.com/openSUSE/rng2doc/pull/61/files

tomschr commented 3 years ago

By the way: Great to see you!

Thanks for the explanation. I thought logging.getLogger("rng2doc")/logging.getLogger(__package__) will return always the same object.

Not always, it depends where you call it. See this StackOverflow article:

"So, for a module located in foo/bar/baz.py, __name__ is set to foo.bar.baz, and __package__ is set to foo.bar, while foo/bar/__init__.py will have foo.bar for both the __name__ and __package__ attributes."

In other words, __package__ can be used to create hierarchical loggers, whereas "rng2doc" will always return the parent logger instance, regardless of the module level.

Hierarchical loggers can be used if you want to distinguish between different modules inside your project. For example, you can configure a transformation and an analyze logger, but with different log levels. One could log into a file, the other prints the message to the console. Just a matter of configuration. :wink:

What advantage do we get importing it from log.py?

The same you get when you split something into a new file: separation. :slightly_smiling_face:

It's probably a matter of taste where you put it. As you need to access your logger in different places of the project, I found it convenient to add it to a dedicated log.py file and import it wherever I need it. That way I can bundle all the necessary setup steps in one file and only import the logger instance.

Additionally, if I want to switch from Python native setup to a INI configuration file setup, I think it's easier to add everything into this file. However, that's my taste and how I prefer to do it. It doesn't mean it's a standard (although I've found it in several projects).

tomschr commented 3 years ago

My tomschr/example-cli-argparse.py gist contains an example of setting up the logger with a dictionary object.

jloehel commented 3 years ago

By the way: Great to see you!

Thanks :-)

I added another commit which bundles the setup and the logger in log.py again.

tomschr commented 3 years ago

I added another commit which bundles the setup and the logger in log.py again.

Looking at your draft, I think the function setup_logging() is unnecessary. Why? Because it's enough to just import the logger instance. I envision something like this:

import logging

logger = logging.getLogger("rng2doc")
_handler = logging.StreamHandler()
_handler.setFormatter(logging.Formatter('[%(levelname)s] %(message)s'))
logger.addHandler(_handler)

As the logger.setLevel(level) call is done after CLI parsing, you don't need that in the above code.

jloehel commented 3 years ago

I have added the function because I wanted to prevent this:

ᐅ ipython
Python 3.6.12 (default, Dec 02 2020, 09:44:23) [GCC]
Type 'copyright', 'credits' or 'license' for more information
IPython 7.16.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import logging

In [2]: logger = logging.getLogger("rng2doc")

In [3]: logger.handlers
Out[3]: []

In [4]: from rng2doc import rng

In [5]: logger.handlers
Out[5]: [<StreamHandler <stderr> (NOTSET)>]

I can add a NullHandler instead.

tomschr commented 3 years ago

I can add a NullHandler instead.

That would be a good idea. :+1: It's also recommended in the Hitchhiker's Guide to Python.