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

Remove NullHandlers to unsuppress lastResort loging handler #1913

Merged

Conversation

n5y
Copy link
Contributor

@n5y n5y commented Apr 20, 2020

This removes the requirement to call logging.basicConfig() to see errors raised in handlers.

Users that already configure logging are unaffected by this change. Users that rely on python default logging configuration will now see warning+ level logs from PTB in stderr.

Suppressing default logging handler is not a good way control whether or not PTB logging is enabled, since users that actually do want to disable PTB logs might need to use basicConfig to configure something else. For this reason I removed all NullHandlers and not just the ones in dispatcher and updater.

@Poolitzer
Copy link
Member

Sounds good to me.

Copy link
Member

tsnoam commented Apr 21, 2020

@Poolitzer Need to be carefully reviewed.
Everything related to logging is very fragile.

Copy link
Member

@tsnoam But in my opinion still a very important fix to have

@n5y
Copy link
Contributor Author

n5y commented Apr 21, 2020

I'm reasonably sure NullHandlers were only needed to avoid "no handlers could be found for logger" error with py2, as lastResort handler was only added in py3.2. Since PTB no longer supports py2 it should be fine to remove them.

@Bibo-Joshi Bibo-Joshi merged commit 5898e1f into python-telegram-bot:master May 3, 2020
@n5y n5y deleted the unsupress-default-logging-handler branch June 7, 2020 04:41
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants