Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Improved logging behavior #216

Merged
merged 1 commit into from
Feb 6, 2023

Conversation

g3n35i5
Copy link
Contributor

@g3n35i5 g3n35i5 commented Feb 5, 2023

This PR improves the logging behavior of ffmpeg-normalize when used via API/python import.

With the current implementation, the logging configuration of ffmpeg-normalize overwrites the root logging behavior even if it's used via python import. For example, the (now deleted) section

if system() not in ("Windows", "cli"):
    logging.addLevelName(logging.ERROR, "�[1;31mERROR�[1;0m")
    logging.addLevelName(logging.WARNING, "�[1;33mWARNING�[1;0m")
    logging.addLevelName(logging.INFO, "�[1;34mINFO�[1;0m")
    logging.addLevelName(logging.DEBUG, "�[1;35mDEBUG�[1;0m")

added the ANSI color codes to all log messages and the entire output of ffmpeg-normalize is put into the log of the application that imports it. However, in most cases the developers might want to configure this behavior.

With this implementation, you can choose whether or whether not to use the logging output of ffmpeg-normalize. To enable the log output, the following code section (or similar) is necessary:

# handler is defined by your application
ffmpeg_logger = logging.getLogger('ffmpeg_normalize')
ffmpeg_logger.addHandler(handler)
ffmpeg_logger.setLevel("DEBUG")

Please note that the CLI logging behavior of ffmpeg-normalize doesn't change with this PR. I'm configuring the logger in the CLI entrypoint in the same way as it was before (with the help of colorlog

@slhck
Copy link
Owner

slhck commented Feb 5, 2023

Thanks! I will have a better look at this tomorrow. Logging had always felt a little bit bolted on, so I really appreciate your input. (In fact I may have to use the same approach in all other packages.)

@g3n35i5
Copy link
Contributor Author

g3n35i5 commented Feb 5, 2023

Thanks! I will have a better look at this tomorrow. Logging had always felt a little bit bolted on, so I really appreciate your input. (In fact I may have to use the same approach in all other packages.)

Glad that I can help! There is certainly still some what you could change / improve the logging. I wanted to make only a minimal improvement with this PR, which does not bring too many changes with it.

@slhck
Copy link
Owner

slhck commented Feb 6, 2023

Merged and released as 1.26.2!

@slhck
Copy link
Owner

slhck commented Feb 8, 2023

Unfortuantely this has broken conda-forge builds: conda-forge/ffmpeg-normalize-feedstock#17

I think I can fix this with the requirements.txt adding the dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants