-
Notifications
You must be signed in to change notification settings - Fork 6k
Fix a bug with non-blocking entry points #3068
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
Conversation
harshil21
left a comment
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.
good catch by the OP
|
I tested out the current release 20.0a0. Every non-blocking callback inside state or fallback that returns None has that issue not only entry points I think. Maybe that is already fixed somewhere else. Currently, when I am returning None in any state I get the PendingState... error. |
|
@CedricFauth thanks for the info! I've pushed more changes that should address that. Could you check if those fix the problems for you? You can install from this branch via |
|
@Bibo-Joshi Yes but no :/ You fixed my previous problem. Returning None now works inside the conversation handler but a new problem occurred: |
|
Yeah, I should have properly run the tests before pushing 🤦♂️ hopefully it's now better :) |
|
Seems to work now, thanks! |
harshil21
left a comment
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.
So I was testing all combinations of block=False/True of ConversationHandler with the handlers in entry_points, states and fallbacks, and noticed that when ConvHandler(..., block=False), the exception thrown from one entry point isn't discarded correctly, and when another entry point is run, the same exception is printed again.
MRE: https://nekobin.com/modoxayexo
Steps to reproduce:
- Run bot, send
/start. You'll seein startprinted along with the exception. - Now send
/start2. You'll see the same exception printed again, and thenin start2is printed to console. - State is correctly switched to
GENDERthough.
Expected behavior:
In step 2, the exception should not be printed again. This is indeed the behaviour when ConvHandler(..., block=True).
Just to double check: your observation is only about error prints & not about wrong behavior right? The difference is the heading of the traceback: The first (triggered by |
Yes. It confused me, thinking it was running the But why should we re-print the error message when it's already printed/forwarded to error handler the first time? Maybe we should move the |
harshil21
left a comment
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.
Did some more testing, and everything seems to run fine now
See https://t.me/pythontelegrambotgroup/595029?thread=595002 for discussion. The problem is that
_update_statedoesn't do anything if the new state isNone(becausereturn Nonemeans "stay in the current state") so that_conversations.get(key)returns thePendingState.Checklist for PRs
.. versionadded:: version,.. versionchanged:: versionor.. deprecated:: versionto the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)AUTHORS.rst(optional)__all__s