Skip to content

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

Merged
tsnoam merged 2 commits into
python-telegram-bot:masterfrom
tobiaswicker:master
Jan 11, 2020
Merged

Fix wrong signature call for ConversationHandler.TIMEOUT handlers (issue #1652)#1653
tsnoam merged 2 commits into
python-telegram-bot:masterfrom
tobiaswicker:master

Conversation

@tobiaswicker

@tobiaswicker tobiaswicker commented Nov 28, 2019

Copy link
Copy Markdown
Contributor

Fix for issue #1652

@tobiaswicker

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@tobiaswicker

Copy link
Copy Markdown
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
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
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
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

@Bibo-Joshi Fair enough

Copy link
Copy Markdown
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

tsnoam commented Jan 11, 2020

Copy link
Copy Markdown
Member

@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
@Bibo-Joshi Bibo-Joshi added 🔌 bug pr description: bug and removed bug 🐛 labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

🔌 bug pr description: bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants