Skip to content
This repository has been archived by the owner on Nov 28, 2022. It is now read-only.

Fixing: Logfile with lower loglevel than console output #61

Closed
wants to merge 5 commits into from

Conversation

LuckyJosh
Copy link

This fixes the issue described in #57.
To sum it up briefly:
The loglevel of a Handler added to a logger object is limited to be equal to or higher than the
loglevel of the logger object. Previously the logger objects loglevel was always set to the loglevel of
the corresponding StreamHandler. This meant that the loglevel a later defined logfile could not be lower.

This 'fix' is the easiest I could come up with and has very minimal changes to the code. It sets the loglevel of the logger object by default to DEBUG, the (by default) lowest logging level.

Previous to this change the logger objects loglevel
was always set to the loglevel of the StreamHandler.
This limited the possible loglevels for logfiles
to be equal to or higher than this loglevel, since
the loglevel of handlers added to the logger object
are limited be the loglevel of the latter.

With the logger object having the lowest possible
loglevel it is now possible to define a logfile
with a lower loglevel (e.g. DEBUG) than the console
output (e.g. INFO).
@@ -112,7 +112,7 @@ def setup_logger(name=None, logfile=None, level=logging.DEBUG, formatter=None, m
"""
_logger = logging.getLogger(name or __name__)
_logger.propagate = False
_logger.setLevel(level)
_logger.setLevel(logging.DEBUG)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid this doesn't work. The user supplies a level and expects it to be used. What exactly does this change improve? By default it is already using logging.DEBUG.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do see the problem you are describing but I think it depends on the point of view of the user.

I personally think of the level as being the loglevel at which the log messages
get displayed on the console, since the StreamHandler gets the same loglevel.
I do not think about the loglevel assigned to the logger object itself, because on its own
it does not do much.
The changes proposed here shift the interpretation of the level parameter slightly.
Currently it is referring to the logger object and the attached StreamHandler. After applying
the changes it is only referring to the loglevel of the StreamHandler.
Fixing the loglevel of the logger object to DEBUG might be unexpected in the sense that
a user might not expect the logger object to be set up this way, but not in the sense that this will
result in unexpected behavior.
Setting the logger objects loglevel to always be the same as the StreamHandlers loglevel (as it is currently done) does however result in unexpected behavior as described in #57 .

The advantage of fixing the logger objects loglevel to DEBUG is that the attached handlers
(presumably a StreamHandler and a FileHandler in most cases) are able to have independent loglevels.

I have thought about this change for a bit, but I came to the conclusion that this change is
in line with the goal of this package to increase the ease of use of the logging module.
Even though this package is just a thin wrapper it feels a bit more “higher level” then the
logging module. One big part of this for me personally is not really having to think about the
logger object itself. It feels in a way more like interacting with the StreamHandler.

A different approach would be to add a new parameter streamLogLevel with similar functionality to existing parameter fileLogLevel. This would in turn require more changes to
the code.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks for the explanation. I will have to think about this and test a little when i find the time the next days. 👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate your consideration, thanks.

@mgiaco
Copy link

mgiaco commented Mar 6, 2018

Any news here?
cheers mathias

@metachris
Copy link
Owner

Thanks for submitting this PR, and with a test too! It is now superseded by #339 therefore closing this. Feel free to reopen if there are any issues or improvements over the other <3

@metachris metachris closed this Oct 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants