search

unoconv / unoserver

MIT License
598 stars 81 forks source link

Do not tamper with the root logger #11

Closed mara004 closed 2 years ago

mara004 commented 3 years ago

Calling logger.basicConfig() is not desirable in this place as it alters the root logger and overrides custom formatters of downstream users.

mara004 commented 2 years ago

This issue is deeper than I expected. After doing some more tests, it seems you also may not do any logger.setLevel(...) calls in the code files, as this will make it impossible for upstream users to set the logging level of unoserver, because the files unoserver.server / unoserver.converter will always be processed last and hence the log level defined there will be effective.

The only solution that comes to my mind would be to move the setLevel() calls into the main methods of the two files, so it will only be applied for CLI users, not for library users, where the configuration defined upstream should be taken.

From the logging documentation:

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.

regebro commented 2 years ago

Sorry, I have been deep down in another complex issue and intentionally not looked at this because I didn't want to be distracted. I'll look at this this week, I hope.

mara004 commented 2 years ago

No problem, take your time. As I stated in the previous comment, this does not fully address the logging issues anyway and I'll have to look into this more thoroughly.

regebro commented 2 years ago

Thanks for this! You are absolutely right, the logging configuration should not be done during import.

I moved the logging configuration to the main() functions, that should solve the issue, so you can still import the library without this changing your logging.

mara004 commented 2 years ago

Thanks!