pconf icon indicating copy to clipboard operation
pconf copied to clipboard

disable default logging

Open ronlut opened this issue 3 years ago • 4 comments

Fixes #34

Proposed Changes

ronlut avatar Jul 13 '21 11:07 ronlut

This will suppress the warning if the user doesn't have a logging facility set up, which I don't think is the proper default behavior.

I'd rather support a solution that would either: a) Allow the logging of these warning to be configured externally b) If there is a logger set up, use that to log these warnings

andrasmaroy avatar Jul 20 '21 19:07 andrasmaroy

@andrasmaroy Following python's library logging guide:

It is strongly advised that you do not add any handlers other than NullHandler to your library’s loggers. This is because the configuration of handlers is the prerogative of the application developer who uses your library. The application developer knows their target audience and what handlers are most appropriate for their application: if you add handlers ‘under the hood’, you might well interfere with their ability to carry out unit tests and deliver logs which suit their requirements.

Also, regarding logging.warning vs warnings.warn:

warnings.warn() in library code if the issue is avoidable and the client application should be modified to eliminate the warning logging.warning() if there is nothing the client application can do about the situation, but the event should still be noted

This is exactly the case for my PR. The warning produced by Pconf is nothing more than a log saying "you tried to load a file but it doesn't exist, so I am ignoring it". I don't think forcing logs is the correct approach, and what I tried doing is exactly that: allowing the user to control whether he wants pconf logs or not

When looking at it again, I think the captureWarnings call is wrong and shouldn't be part of the library, as this affect the client's application.

WDYT about changing all those warnings to logger.warning or even logger.info? Then each application can choose whether it wants those warnings or not.

ronlut avatar Jul 21 '21 16:07 ronlut

Thank you @ronlut for taking the time for the thorough description and referencing sources!

Yes we are on the same page on this topic, I don't want to set handlers in the library, the guideline is absolutely right on that topic.

My issue was exactly as you said with captureWarnings as that resulted in the warnings disappearing if one doesn't have a logger set up.

Changing the warnings the warnings to logger.warning should be good, I think it aligns with the guide as in this case its not the application itself that should be modified necessarily to remedy the warning, but rather the environment its ran in.

andrasmaroy avatar Jul 22 '21 09:07 andrasmaroy

@andrasmaroy removed the capturing + changed to logger. Now, for the following code:

import logging
from pconf import Pconf

Pconf.file("non_existing_file_1")
logging.basicConfig(level=logging.INFO, format='%(asctime)s %(levelname)-8s %(name)-30s %(message)s')
Pconf.file("non_existing_file_2")

The output is:

2021-07-22 13:43:23,362 WARNING  pconf.store.file               IOError when opening non_existing_file_2

ronlut avatar Jul 22 '21 10:07 ronlut