search

sylikc / pyexiftool

PyExifTool (active PyPI project) - A Python library to communicate with an instance of Phil Harvey's ExifTool command-line application. Runs one process with special -stay_open flag, and pipes data to/from. Much more efficient than running a subprocess for each command!
Other
161 stars 21 forks source link

Don't use the root logger #24

Closed nyoungstudios closed 3 years ago

nyoungstudios commented 3 years ago

Sets logger name rather than using the root logger. I think it is good practice to not use the root logger in a Python library since it can cause logging problems with other library's loggers (since all loggers inherit from the root logger).

jangop commented 3 years ago

This might not be the proper place to ask, but what are your thoughts on using loguru? I like it sooooo much better, and it works for libraries: https://pypi.org/project/loguru/#suitable-for-scripts-and-libraries

nyoungstudios commented 3 years ago

loguru looks like a great library for logging, but I don't think it is a good idea to use it in this situation because this is a relatively simple Python library with only two logging debug statements. Additional, there is no need to make the user install an additional Python package so that they can use this one

sylikc commented 3 years ago

I might merge this as is... the https://github.com/sylikc/pyexiftool/tree/v0.5.x-py3-refactor branch does away with this and allows you to pass in your own pre-configured logger... which can be anything if it's a drop in replacement for Python's logger

though I might have to change that line of code that error checks

elif not isinstance(new_logger, logging.Logger):
    raise TypeError("logger needs to be of type logging.Logger")
nyoungstudios commented 3 years ago

Glad to see that this is done away with in the new refactor branch. I would say this is more of a bug fix than an enhancement, so I think it would be a good idea to merge this first and then the new refactor branch can always be merged whenever it is ready. As for changing that if statement to support other types of loggers, I am not sure if that is recommended as you'll still have to require those other logger types as dependencies. If including other types of loggers is a necessity, then maybe you have this additional requirements installed something like this: pip install pyexiftool[logging] which would be a variant of the package supporting additional logging. This way, if someone knows they don't want the logging functionality, they don't have to install it.

sylikc commented 3 years ago

@nyoungstudios it took awhile ... but now I merged the changes. Do you need a PyPI release?

@jangop loguru looks interesting, and I sort of changed the way the .logger() checks whether it's a valid logger

sylikc commented 3 years ago

A PyPI release was made, and the change is now live