-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Fix #1366: _trigger_timeout() missing 1 required positional argument: 'job' #1367
Conversation
Looking at it, it looks like this might be more complicated than I thought at first, due to how we support both context based and the old style in this version. So this function would need to accept 1 potitional arg and one optional, and then check if arg 1 is of type CallbackContext and do this new logic but otherwise fall back on the old logic. Oh and if you could fix this flake8 error that would be great :) https://travis-ci.org/python-telegram-bot/python-telegram-bot/jobs/508565290#L661 |
@jsmnbom Alright I've made the changes you requested and fixed the 4 pytest failures and flake8 error. Two points to note though:
|
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.
This looks really good now! ^ ^
- I think this naming is fine tbh, please see comment below though
- From what I can gather in that log, it looks like the context that we get in the _trigger_timeout is an entirely different type that other handlers understand, so what you've done now is indeed correct :D
self.logger.debug('conversation timeout was triggered!') | ||
del self.timeout_jobs[job.context.conversation_key] | ||
if isinstance(context, CallbackContext): |
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.
Could you add a comment explaining why we are doing all this mess in the first place? :)
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.
Alright! I've done it, do let me know what you think.
CI fails are unrelated afaik. So this looks good now. Thanks a ton @loozhengyuan ! |
can anyone give an example of how to use |
Fix #1366
Tried fixing this but its my first time so not sure if I got these right. Feel free to let me know what changes needs to be made.