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

Revise the logger instanciation/initial handlers #135

Merged
merged 17 commits into from Nov 24, 2021

Conversation

nmaynes
Copy link
Contributor

@nmaynes nmaynes commented Nov 9, 2021

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 in api.py where if explain 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

Update api.py to preserve backwards compatability with recommended logging practices for python libraries
Copy link
Owner

@Ousret Ousret left a 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.

charset_normalizer/__init__.py Outdated Show resolved Hide resolved
charset_normalizer/api.py Outdated Show resolved Hide resolved
tests/test_logging.py Show resolved Hide resolved
charset_normalizer/__init__.py Outdated Show resolved Hide resolved
charset_normalizer/api.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2021

Codecov Report

Merging #135 (c3970d4) into master (6480728) will decrease coverage by 0.03%.
The diff coverage is 91.30%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
charset_normalizer/api.py 88.09% <84.61%> (-0.69%) ⬇️
charset_normalizer/__init__.py 100.00% <100.00%> (ø)
charset_normalizer/utils.py 85.02% <100.00%> (+0.58%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6480728...c3970d4. Read the comment docs.

@Ousret
Copy link
Owner

Ousret commented Nov 17, 2021

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?

@nmaynes
Copy link
Contributor Author

nmaynes commented Nov 18, 2021

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 explain is set to False should be no stream logger set. Using the NullHandler will allow messages of WARNING level or higher to be pushed to stderr on most systems. One thing I would like to add is a note in the documentation that describes the loggers behavior. How do you feel about the default behavior being set in that manner? And sorry for the slow movement on my part.

@Ousret
Copy link
Owner

Ousret commented Nov 24, 2021

No problems about the delay.

Using the NullHandler will allow messages of WARNING level or higher to be pushed to stderr on most systems.

I don't see how this can happen. Maybe I misunderstood? https://docs.python.org/3/library/logging.handlers.html#logging.NullHandler

when not set and explain is set to False should be no stream logger set

I think that we should give up on using set_stream_handler in the function from_bytes(...), we should rather have a unique handler instantiated once and added/removed from handlers. That does not mean we remove the set_stream_handler.

Lastly, the documentation should be updated to reflect those changes and explain a bit more what we are doing from the begining.

Copy link
Owner

@Ousret Ousret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now

@Ousret Ousret changed the title Enhancement/#134 Revise the logger instanciation/initial handlers Nov 24, 2021
@Ousret
Copy link
Owner

Ousret commented Nov 24, 2021

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.

@nmaynes
Copy link
Contributor Author

nmaynes commented Nov 25, 2021

Thank you for merging these changes!

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.

[Proposal] Revise the logger instanciation/initial handlers
3 participants