Don't suppress messages when logging is not configured#1813
Merged
Byron merged 4 commits intogitpython-developers:mainfrom Jan 26, 2024
Merged
Don't suppress messages when logging is not configured#1813Byron merged 4 commits intogitpython-developers:mainfrom
Byron merged 4 commits intogitpython-developers:mainfrom
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1806
This stops adding
NullHandlerinstances to GitPython's loggers. As noted in #1806, when they were added in #300 this prevented errors when GitPython logged messages and logging was not enabled, but since Python 3.2 there is a logger of last resort providing a nicer default behavior of showing the messages. (They are still shown with better formatting if logging is configured, even if just done withlogging.basicConfig(), so applications should still typically configure logging.)This also makes a few more minor related changes, detailed in the commit messages. This includes changing variable names from
logto_logger, as mentioned in #1806 (comment), as well as consistently passing__name__tologging.getLoggerinstead of hard-coding names in a few places, as mentioned in #1808 (comment).The major effect of the changes here is that messages of level
WARNINGor higher are not suppressed by default anymore, even when logging is not configured. So I could have included a change here to take care of #1808 as well, such as dc62096. But I plan to introduce that in a future pull request instead, because it would be best to add tests for it, which I think should build on the tests proposed in #1812. If both #1812 and this PR are merged, I can add tests followed by the changes from dc62096 that would make them pass. (If they are not merged, or they are merged with major changes, then that will also clarify how to move forward with making a warning from the initial refresh use the logging framework.)This shows as resolving several CodeQL alerts and also adding new alerts that correspond exactly to them. This happens as a result of renaming
logto_logger; CodeQL considers the alert resolved because there are no more calls tolog, and issues new equivalent alerts for the same situations with_logger. The new alerts produce a failing Code scanning results check for the PR.