Skip to content

Added conversation timeout in ConversationHandler#895

Merged
tsnoam merged 4 commits intopython-telegram-bot:masterfrom
evgfilim1:conversation-timeout
Mar 1, 2018
Merged

Added conversation timeout in ConversationHandler#895
tsnoam merged 4 commits intopython-telegram-bot:masterfrom
evgfilim1:conversation-timeout

Conversation

@evgfilim1
Copy link
Contributor

@evgfilim1 evgfilim1 commented Oct 31, 2017 •
edited
Loading

Closes #648

Short usage:

updater = Updater(TOKEN)
conversation = ConversationHandler(
    [CommandHandler('start', start)], 
    {0: MessageHandler(Filters.text, continue_)},
    conversation_timeout=60
)
updater.dispatcher.add_handler(conversation)

@codecov
Copy link

codecov bot commented Nov 1, 2017 •
edited
Loading

Codecov Report

Merging #895 into master will decrease coverage by 0.42%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #895      +/-   ##
==========================================
- Coverage   92.25%   91.82%   -0.43%     
==========================================
  Files         103      103              
  Lines        4169     4060     -109     
  Branches      669      641      -28     
==========================================
- Hits         3846     3728     -118     
- Misses        188      193       +5     
- Partials      135      139       +4
Impacted Files Coverage Δ
telegram/ext/conversationhandler.py 81.81% <100%> (+0.73%) ⬆️
telegram/files/file.py 74.07% <0%> (-14.82%) ⬇️
telegram/utils/helpers.py 82.35% <0%> (-3.7%) ⬇️
telegram/update.py 97.61% <0%> (-2.39%) ⬇️
telegram/user.py 86.36% <0%> (-1.97%) ⬇️
telegram/utils/request.py 67.85% <0%> (-1.79%) ⬇️
telegram/ext/updater.py 77.17% <0%> (-1.16%) ⬇️
telegram/ext/jobqueue.py 91.97% <0%> (-0.88%) ⬇️
telegram/files/inputmediavideo.py 95.23% <0%> (-0.77%) ⬇️
... and 34 more

Eldinnie
Eldinnie approved these changes Nov 9, 2017
View reviewed changes
Copy link
Member

@Eldinnie Eldinnie left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@Eldinnie
Copy link
Member

Eldinnie commented Nov 9, 2017

Looks good to me.

@Eldinnie
Copy link
Member

Eldinnie commented Dec 5, 2017

Anyone else wants a look on this? @tsnoam or @jh0ker ?
Or I'll add pending_merge

@Eldinnie Eldinnie requested a review from jh0ker December 7, 2017 12:06
jh0ker
jh0ker requested changes Dec 7, 2017
View reviewed changes
telegram/ext/conversationhandler.py Outdated
new_state = self.current_handler.handle_update(update, dispatcher)

if self.timeout_job is not None:
self.timeout_job.schedule_removal()
Copy link
Member

@jh0ker jh0ker Dec 7, 2017

Choose a reason for hiding this comment

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

Do I understand correctly that there is only a single timeout job which gets removed whenever any conversation is triggered?

Wouldn't you want to use a different job for each conversation?

Copy link
Contributor Author

@evgfilim1 evgfilim1 Jan 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This timeout job is triggered in any conversation state, if you mean this. For different ConversationHandler instances the job is different too.

UPD. Yes, you're right, I have to use a different job.

telegram/ext/conversationhandler.py Outdated
per_user=True,
per_message=False):
per_message=False,
conversation_timeout=0):
Copy link
Member

@jh0ker jh0ker Dec 7, 2017

Choose a reason for hiding this comment

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

I prefer None as the default value for no timeout

Copy link
Contributor Author

@evgfilim1 evgfilim1 Jan 14, 2018

Choose a reason for hiding this comment

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

I'll change it, but there is no difference, I think.

tests/test_conversationhandler.py
update = Update(0, message=message)
assert not handler.check_update(update)

def test_conversation_timeout(self, dp, bot, user1):
Copy link
Member

@jh0ker jh0ker Dec 7, 2017

Choose a reason for hiding this comment

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

Related to the comment above, perhaps adding a test for 2 users would be prudent.

@jsmnbom jsmnbom added the 📋 pending-reply work status: pending-reply label Dec 29, 2017
@evgfilim1
Copy link
Contributor Author

evgfilim1 commented Jan 15, 2018 •
edited
Loading

@jh0ker Done

UPD. This branch is based on an old revision of master branch, so some checks can fail. I can re-open PR to follow latest revision of master, if it is necessary or important.

@Eldinnie
Copy link
Member

Eldinnie commented Jan 18, 2018

Hi, because you're latest commit has [ci skip] the latest version is not run on CI's and codecov. Could rebase on current master and commit so it will run a full CI?

Thanks

@evgfilim1
Copy link
Contributor Author

evgfilim1 commented Jan 19, 2018 •
edited
Loading

@Eldinnie I know, I marked it because there are no useful changes (I removed a line with comment that is not needed at all)

@evgfilim1
Copy link
Contributor Author

evgfilim1 commented Jan 20, 2018

Maybe we remove "pending reply" label?

@tsnoam
Copy link
Member

tsnoam commented Jan 20, 2018

@evgfilim1
the last commit was marked "ci skip" and the commit before it failed unitests.
would you please be so kind to rebase and remove the "ci skip" from the commit message?

@evgfilim1
Copy link
Contributor Author

evgfilim1 commented Jan 21, 2018 •
edited
Loading

@tsnoam done.
CI failed because of test_official[sendInvoice]

@evgfilim1
Copy link
Contributor Author

evgfilim1 commented Jan 29, 2018

test_official fail status should be solved when #986 is merged.

@tsnoam tsnoam merged commit 811369d into python-telegram-bot:master Mar 1, 2018
@tsnoam
Copy link
Member

tsnoam commented Mar 1, 2018

@evgfilim1 Thank you for your contribution

@evgfilim1 evgfilim1 deleted the conversation-timeout branch March 2, 2018 17:51
This was referenced Mar 2, 2018
Closed
Closed
Merged
Merged
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Aug 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jh0ker jh0ker jh0ker requested changes

@Eldinnie Eldinnie Eldinnie approved these changes

Assignees

No one assigned

Labels

📋 pending-reply work status: pending-reply

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

how to set conversation no-operation timeout

5 participants

@evgfilim1 @Eldinnie @tsnoam @jh0ker @jsmnbom