Skip to content

Fix #1366: _trigger_timeout() missing 1 required positional argument: 'job'#1367

Merged
jsmnbom merged 2 commits intopython-telegram-bot:masterfrom
loozhengyuan:fix-1366
Apr 5, 2019
Merged

Fix #1366: _trigger_timeout() missing 1 required positional argument: 'job'#1367
jsmnbom merged 2 commits intopython-telegram-bot:masterfrom
loozhengyuan:fix-1366

Conversation

@loozhengyuan
Copy link
Contributor

@loozhengyuan loozhengyuan commented Mar 19, 2019 •
edited
Loading

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.

jsmnbom
jsmnbom requested changes Mar 19, 2019
View reviewed changes
@jsmnbom jsmnbom self-requested a review March 19, 2019 19:50
loozhengyuan
loozhengyuan commented Mar 19, 2019
View reviewed changes
@jsmnbom
Copy link
Member

jsmnbom commented Mar 19, 2019

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.
That's what I'm getting from the test faliures anyways... Just say if you need any help with doing that or if you'd rather let one of us try ^^

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

@loozhengyuan
Copy link
Contributor Author

loozhengyuan commented Mar 20, 2019 •
edited
Loading

@jsmnbom Alright I've made the changes you requested and fixed the 4 pytest failures and flake8 error. Two points to note though:

  1. I am not sure if i named the arguments correctly (maybe job needs to be changed to something else?), so let me know what should be changed.

  2. I didn't include context=context on handler.handle_update because it failed one of the tests. I have included the log below:

_____________________________________________________________________ TestConversationHandler.test_conversation_handler_timeout_state ______________________________________________________________________

self = <tests.test_conversationhandler.TestConversationHandler object at 0x10b635048>, dp = <telegram.ext.dispatcher.Dispatcher object at 0x10b5ed7b8>, bot = <telegram.bot.Bot object at 0x10afa4f98>
user1 = <telegram.user.User object at 0x10b5f0320>

    def test_conversation_handler_timeout_state(self, dp, bot, user1):
        states = self.states
        states.update({ConversationHandler.TIMEOUT: [
            CommandHandler('brew', self.passout),
            MessageHandler(~Filters.regex('oding'), self.passout2)
        ]})
        handler = ConversationHandler(entry_points=self.entry_points, states=states,
                                      fallbacks=self.fallbacks, conversation_timeout=0.5)
        dp.add_handler(handler)
    
        # CommandHandler timeout
        message = Message(0, user1, None, self.group, text='/start',
                          entities=[MessageEntity(type=MessageEntity.BOT_COMMAND, offset=0,
                                                  length=len('/start'))],
                          bot=bot)
        dp.process_update(Update(update_id=0, message=message))
        message.text = '/brew'
        message.entities[0].length = len('/brew')
        dp.process_update(Update(update_id=0, message=message))
        sleep(0.5)
        dp.job_queue.tick()
>       assert handler.conversations.get((self.group.id, user1.id)) is None
E       assert 1 is None
E        +  where 1 = <built-in method get of dict object at 0x10b67f4c8>((0, 123))
E        +    where <built-in method get of dict object at 0x10b67f4c8> = {(0, 123): 1}.get
E        +      where {(0, 123): 1} = <telegram.ext.conversationhandler.ConversationHandler object at 0x10b635b38>.conversations

tests/test_conversationhandler.py:531: AssertionError
-------------------------------------------------------------------------------------------- Captured log call ---------------------------------------------------------------------------------------------
jobqueue.py                264 ERROR    An uncaught error was raised while executing job _trigger_timeout
Traceback (most recent call last):
  File ".../python-telegram-bot/telegram/ext/jobqueue.py", line 260, in tick
    job.run(self._dispatcher)
  File ".../python-telegram-bot/telegram/ext/jobqueue.py", line 390, in run
    self.callback(dispatcher.bot, self)
  File ".../python-telegram-bot/telegram/ext/conversationhandler.py", line 368, in _trigger_timeout
    handler.handle_update(context.update, context.dispatcher, check, context=context)
  File ".../python-telegram-bot/telegram/ext/handler.py", line 117, in handle_update
    return self.callback(update, context)
  File ".../python-telegram-bot/tests/test_conversationhandler.py", line 100, in passout
    assert update.message.text == '/brew'
AttributeError: '_ConversationTimeoutContext' object has no attribute 'message'

jsmnbom
jsmnbom requested changes Apr 4, 2019
View reviewed changes
Copy link
Member

@jsmnbom jsmnbom left a 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! ^ ^

  1. I think this naming is fine tbh, please see comment below though
  2. 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

telegram/ext/conversationhandler.py
def _trigger_timeout(self, context, job=None):
self.logger.debug('conversation timeout was triggered!')
del self.timeout_jobs[job.context.conversation_key]
if isinstance(context, CallbackContext):
Copy link
Member

@jsmnbom jsmnbom Apr 4, 2019

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? :)

Copy link
Contributor Author

@loozhengyuan loozhengyuan Apr 5, 2019

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.

@jsmnbom
Copy link
Member

jsmnbom commented Apr 5, 2019

CI fails are unrelated afaik. So this looks good now. Thanks a ton @loozhengyuan !

@jsmnbom jsmnbom merged commit 2ed4cbd into python-telegram-bot:master Apr 5, 2019
@A-Iskakov
Copy link

A-Iskakov commented Jun 22, 2019

can anyone give an example of how to use ConversationHandler with conversation_timeout?

@github-actions github-actions bot locked and limited conversation to collaborators Aug 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jsmnbom jsmnbom jsmnbom approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

_trigger_timeout() missing 1 required positional argument: 'job'

3 participants

@loozhengyuan @jsmnbom @A-Iskakov