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 unresolvable promises #1270

Merged
merged 7 commits into from Feb 14, 2019

Conversation

@vasinkd
Copy link
Contributor

vasinkd commented Oct 23, 2018

Situation:
We use ConversationHandler with run_async decorator and an entry_point function returns None

Result:
We stuck within conversation state (None, Promise) since Promise returns None and None means we do not update conversation state.

Solution:
If Promise returns None AND old state is None - exit Conversation Handler.

Caveats:
May be not suitable if a programmer uses None as a valid state in Conversation states dict. But why would anyone do that?

@jh0ker jh0ker added the bug 🐛 label Oct 26, 2018
@jh0ker

This comment has been minimized.

Copy link
Member

jh0ker commented Oct 26, 2018

Good catch, thanks for your contribution!

This also seems to be the behavior for when a synchronous entry point handler returns None, although this is never explicitly documented. Perhaps it would be a good idea to do so.

It would also be great if you could add a unit test that tests for this bug, so we won't have any regressions in the future. It should probably look similar to this test.

Edit: I don't think the caveat you mentioned is real, pretty sure it is already not possible to use None for states

Copy link
Member

jh0ker left a comment

Needs a test and documentation

@jsmnbom

This comment has been minimized.

Copy link
Member

jsmnbom commented Jan 4, 2019

@vasinkd is test and docs something you would like to do an attempt at?

@vasinkd

This comment has been minimized.

Copy link
Contributor Author

vasinkd commented Jan 6, 2019

Totally forgot about this. @jsmnbom , thanks for a reminder!

Ok, I'll add tests, no problemo.
What about documentation? Should I mention this behavior in ConversationHandler class description?

@jsmnbom

This comment has been minimized.

Copy link
Member

jsmnbom commented Jan 6, 2019

Thanks a lot :D
In terms of docs I was mostly referring to @jh0ker's comment about

This also seems to be the behavior for when a synchronous entry point handler returns None, although this is never explicitly documented. Perhaps it would be a good idea to do so.

So a note in the ConversationHandler class docstring about this behaviour (both sync and async) sounds good yea :)

@vasinkd

This comment has been minimized.

Copy link
Contributor Author

vasinkd commented Jan 27, 2019

Done. Sorry for the delay!
Better late than never :)

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Feb 13, 2019

Test for this instance passed. Can we merge for V12 @jsmnbom ?

Copy link
Member

jsmnbom left a comment

I see what you mean. Yea let's merge it :)

@Eldinnie Eldinnie changed the base branch from master to V12 Feb 14, 2019
@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Feb 14, 2019

CI is running, should be able to be merged in v12

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Feb 14, 2019

Merged V12 after making small adjustment there regarding conversationhandler. If CI agrees we can merge.

jsmnbom added a commit that referenced this pull request Feb 14, 2019
Also include #1270 even though not merged yet, but it should be very soon :)
@Eldinnie Eldinnie dismissed jh0ker’s stale review Feb 14, 2019

not reviewed since last changes

@Eldinnie Eldinnie merged commit 60f2044 into python-telegram-bot:V12 Feb 14, 2019
3 of 4 checks passed
3 of 4 checks passed
codecov/patch 0% of diff hit (target 92.79%)
Details
codecov/project 93.05% (+0.26%) compared to cd7c642
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.