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

[Proposal] Revise the logger instanciation/initial handlers #134

Closed
nmaynes opened this issue Nov 4, 2021 · 2 comments · Fixed by #135
Closed

[Proposal] Revise the logger instanciation/initial handlers #134

nmaynes opened this issue Nov 4, 2021 · 2 comments · Fixed by #135
Labels
enhancement New feature or request

Comments

@nmaynes
Copy link
Contributor

nmaynes commented Nov 4, 2021

Is your feature request related to a problem? Please describe.
The Logger is initialized in charset/api.py in a way that does not allow developers using the library to change the logging level via the root logger. The Python Logging library now makes a NullHandler available to allow library and package developers to manage logging in a way that is flexible for application developers. The (Python 3 documentation)[https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library] has this snippet:

It is strongly advised that you do not add any handlers other than NullHandler to your library’s loggers. This is because the configuration of handlers is the prerogative of the application developer who uses your library. The application developer knows their target audience and what handlers are most appropriate for their application: if you add handlers ‘under the hood’, you might well interfere with their ability to carry out unit tests and deliver logs which suit their requirements.

Describe the solution you'd like
I would like to change api.py to set up a NullHandler logger and add a function to allow application developers to set the StreamHandler. The function would be added to the init.py file for charset_normalizer. The existing format would be provided as the default for the StreamHandler. The boto3 library has a nice example of this. Including the function below.

def set_stream_logger(
          name="charset_normalizer",
          level=logging.INFO,
          format_string="%(asctime)s | %(levelname)s | %(message)s",
      ):
  
          logger = logging.getLogger(name)
          logger.setLevel(level)
          handler = logging.StreamHandler()
          handler.setLevel(level)
          formatter = logging.Formatter(format_string)
          handler.setFormatter(formatter)
          logger.addHandler(handler)

Additional context
I am happy to write the PR for this. Just wanted to make sure the developers have not considered this and dismissed the change for a reason unbeknownst to me. Also, please let me know if this is not a good fit for the library. I won't be offended! I promise :)

@nmaynes nmaynes added the enhancement New feature or request label Nov 4, 2021
@Ousret
Copy link
Collaborator

Ousret commented Nov 6, 2021

Hi,

I am open to that. Provided that the argument explain: bool = False does act like intended for those who are used to it.
Thanks for the detailed proposal.

To resume the proposal and add a bit of constraint.

  • Initializing the logger with a NullHandler by default in __init__.py
  • Creating a function that would enable the StreamHandler like setup nowadays eg. fn set_stream_logger
  • When explain: bool = True toggle is set, should behave like before

Just wanted to make sure the developers have not considered this and dismissed the change for a reason unbeknownst to me.

Was not dismissed, I focused the work on other matters at hand.

@nmaynes
Copy link
Contributor Author

nmaynes commented Nov 6, 2021

Thanks! I'll draft a PR over the next couple of days that meets these conditions.

@Ousret Ousret changed the title [Proposal] [Proposal] Revise the logger instanciation/initial handlers Nov 8, 2021
Ousret added a commit to nmaynes/charset_normalizer that referenced this issue Nov 20, 2021
Ousret added a commit to nmaynes/charset_normalizer that referenced this issue Nov 21, 2021
Ousret added a commit to nmaynes/charset_normalizer that referenced this issue Nov 24, 2021
Ousret added a commit to nmaynes/charset_normalizer that referenced this issue Nov 24, 2021
Ousret added a commit that referenced this issue Jan 29, 2022
Ousret added a commit that referenced this issue Jan 29, 2022
* 🔧 The logging behavior have been completely reviewed, now using only TRACE and DEBUG levels

Following #162 and #145 and #134
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants