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 error handlers from examples #1983

Merged

Conversation

n5y
Copy link
Contributor

@n5y n5y commented Jun 7, 2020

This should have been done a long time ago. A new example for a custom error handler can be added later.

Closes #1921

Most examples use the same error handler, that error handler logs
update.to_dict but doesn't log error traceback. Hiding error traceback
is quite bad, removing the error handler entirely causes PTB to use
default error logging which does include error traceback.
Copy link
Member

@n5y Kind of related to our refactoring to drop the need to import and call logging. I wanted to get (up) to (date on) that, soonTM

@Bibo-Joshi
Copy link
Member

Hi. Thanks for the PR. After short internal discussion we'd like to add a dedicated error handler example to the examples section in the same move. Would you care to add one? You can base it on the snippet in the wiki.

@n5y
Copy link
Contributor Author

n5y commented Jun 7, 2020

I don't think lack of custom error handler example justifies keeping traceback-hiding error handlers in the examples any longer. Especially if there is a wiki entry about custom error handlers.

I also don't think the wiki snippet can be used as is. It's rather complex, but lacks some required safeguards and can hide original exception with a new one. It re-rises exception at the end which I don't think works too well with how PTB handles this. It uses f-string which are not supported by python 3.5.

Perhaps the snippet can just continue to live in the wiki where it can keep f-strings and is easier to modify. It's also easier to show multiple handler variants on the wiki. Or perhaps we can make another feature request issue specifically for an error handler example.

@Poolitzer
Copy link
Member

Yeah the snippet isnt perfect, agreed. Nonetheless, a simple error handler example isnt too hard to do, if you dont want to, I will happily throw one together.

Im not sure I get It re-rises exception at the end which I dont think works too well with how PTB handles this. This re raising works perfectly for me.

@Poolitzer
Copy link
Member

okay I added a simple error handler imo, is that ok?

n5y added 3 commits June 8, 2020 09:28
Including:
- Change the telegram message to include usual python error message.
- HTML-escape the strings used to build the telegram message.
- Capitalize comments and add more empty lines to hopefully unify the
  style with other examples, at least a bit.
- Reorder imports.
Copy link
Member

@Bibo-Joshi Bibo-Joshi 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 update! I left two minor comments on the error_handler example.
Also I'd prefer consistent naming of the examples, e.g. errorhandlerbot.py

examples/error_handler.py Outdated Show resolved Hide resolved
examples/error_handler.py Outdated Show resolved Hide resolved
examples/errorhandlerbot.py Outdated Show resolved Hide resolved
n5y added 3 commits June 12, 2020 13:42
Otherwise the example will require two edits 40 lines apart to run.
The example requires you to set developer chat id, this change will
make things easier for users that don't know how to see their chat id.
@n5y
Copy link
Contributor Author

n5y commented Jun 12, 2020

I also added chat id in start command response to make it easier for people who don't know how to see their chat id.

@n5y
Copy link
Contributor Author

n5y commented Jun 12, 2020

Should I squash this branch into a single "Remove error handlers from examples and add a dedicated error handler example" commit? And reabase it onto current master.

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Jun 12, 2020

Should I squash this branch into a single "Remove error handlers from examples and add a dedicated error handler example" commit? And reabase it onto current master.

No need. I'll squash on merge. LGTM now.

Edit: I forgot about the Readme in the examples directory. Please list the new example there.

@n5y
Copy link
Contributor Author

n5y commented Jun 12, 2020

There are a few other examples missing from the readme, but I think it should be fixed in a separate PR, rather than this one.

@Bibo-Joshi
Copy link
Member

There are a few other examples missing from the readme, but I think it should be fixed in a separate PR, rather than this one.

Thanks for pointing that out. Updated right now, giving you a MC ;P. But @Poolitzer wants to review anyway.

Copy link
Member

@Poolitzer Poolitzer left a comment

Choose a reason for hiding this comment

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

LGTM, I would change the text in the ReadMe, but maybe thats just me

examples/README.md Outdated Show resolved Hide resolved
@Bibo-Joshi Bibo-Joshi merged commit a4e78f6 into python-telegram-bot:master Jun 12, 2020
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Remove error handlers from examples
3 participants