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

Fix wrong signature call for ConversationHandler.TIMEOUT handlers (issue #1652) #1653

Merged
merged 2 commits into from
Jan 11, 2020

Conversation

tobiaswicker
Copy link
Contributor

@tobiaswicker tobiaswicker commented Nov 28, 2019

Fix for issue #1652

@tobiaswicker
Copy link
Contributor Author

Forced push was necessary to trigger the checks to run again, because one failed for unrelated reasons.

@Bibo-Joshi Bibo-Joshi added this to the 12.5 milestone Nov 29, 2019
@Eldinnie
Copy link
Member

`Thanks for making this fix.
Can we explore adding a test to make sure this (keeps) work(ing/s) as intended?

@Eldinnie Eldinnie modified the milestones: 12.5, 12.3 Dec 10, 2019
@tobiaswicker
Copy link
Contributor Author

@Eldinnie PTB does have tests for the ConversationHandler and there is even a test_conversation_handler_timeout_state() test case that specifically tests the ConversationHandler.TIMEOUT state. I think the issue you are talking about, is that those tests don't use_context, right?

So would you like to see the existing tests migrated to use context or rather have separate tests? I'd opt for the latter for backward compatibility for now.

Copy link
Member

@tobiaswicker Switching all the tests to contexts is actually an issue, #1509

@tobiaswicker
Copy link
Contributor Author

@Poolitzer OK, thanks!

@Eldinnie does the mentioned issue satisfy your request for a test or have you had a test in mind that wouldn't be covered by migrating any of the existing tests?

Copy link
Member

@tobiaswicker I am very sure that solving that issue will satisfy his testing needs

Copy link
Member

@Poolitzer Note, that #1509 is currently assigned only to the v13,0 milestone, which will probably not be closed too soon.

Copy link
Member

@Bibo-Joshi I mean, we are talking about a test for a callback here. I dont think it will break soon enough that we will have a problem with adding this test later, do you?

Copy link
Member

@Poolitzer I guess not. Just wanted to mention it ;)

Copy link
Member

@Bibo-Joshi Fair enough

Copy link
Member

@tobiaswicker IMO the new PR should be paired with a test. One that would fail without this fix, and will pass with this fix.

Bibo-Joshi added a commit that referenced this pull request Jan 10, 2020
@tsnoam tsnoam merged commit 940b42e into python-telegram-bot:master Jan 11, 2020
@tsnoam
Copy link
Member

tsnoam commented Jan 11, 2020

@tobiaswicker thankyou for your contribution

tsnoam pushed a commit that referenced this pull request Jan 11, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Aug 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants