-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
Revise the logger instanciation/initial handlers #135
Conversation
Update api.py to preserve backwards compatability with recommended logging practices for python libraries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the submission. I have assembled some comments/remarks around the proposed changes.
Codecov Report
@@ Coverage Diff @@
## master #135 +/- ##
==========================================
- Coverage 89.66% 89.62% -0.04%
==========================================
Files 11 11
Lines 1171 1186 +15
==========================================
+ Hits 1050 1063 +13
- Misses 121 123 +2
Continue to review full report at Codecov.
|
I can see that you marked some comments as resolved but did not see any commits to refer to. Do you need any help to pursue this PR? Or is it just the lack of free time? |
It is the time that is missing. I do have a commit that addresses all but one of the comments. I think the default behavior of the logger, when not set and |
Move logger into utils.py Add test cases for stream handler behavior
No problems about the delay.
I don't see how this can happen. Maybe I misunderstood? https://docs.python.org/3/library/logging.handlers.html#logging.NullHandler
I think that we should give up on using Lastly, the documentation should be updated to reflect those changes and explain a bit more what we are doing from the begining. |
instead of utils.py
about explain=True behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now
Hopefully, the principal concern is now answered. I am going forward and merging this, do not hesitate to review it and notify me in case I missed something. |
Thank you for merging these changes! |
I added the logging functionality described in the proposal. I also took care to make sure the
explain
argument would operate the same way. I left the behavior inapi.py
where ifexplain
is not set, the logger will still log messages at the WARNING level. That behavior is really up to you as the package maintainer. It is as easy as removing that branch from the if statement and adding documentation to the repository that describes how a logger must be set via the handler if an application developer so desires.I also added two simple tests that check whether the
set_stream_handler
function does what it should. Apologies if the tests are not in the correct style. Let me know if anything is in need of attention or you have changed your mind about the behavior change for logging. Thanks for the awesome library.Close #134