Skip to content

Add run_async parameter to ConversationHandler#2292

Merged
Bibo-Joshi merged 7 commits intopython-telegram-bot:masterfrom
zeshuaro:conv-handler-async-param
Jan 9, 2021
Merged

Add run_async parameter to ConversationHandler#2292
Bibo-Joshi merged 7 commits intopython-telegram-bot:masterfrom
zeshuaro:conv-handler-async-param

Conversation

@zeshuaro
Copy link
Contributor

@zeshuaro zeshuaro commented Jan 6, 2021

Bibo-Joshi
Bibo-Joshi requested changes Jan 6, 2021
View reviewed changes
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your PR!
Looking at the changes, I was wondering if maybe it's a better idea to not override the setting of the handlers but only change it during handle_update, i.e. something along the lines

setting = handler.rus_async
handler.run_async = True if self.run_async else handler.run_async
handler.handle_updater(update)
handler.run_async = setting

but I'm not 100% sure if that could still have side effects if the same handler object is used in two places … Currently Dispatcher.process_update is calling handle_update() one after the other, even if internally they might call Dispatcher.run_async but if that's changed at some point and the above approach will be hard to debug. and maybe the use case for having the same handler object in multiple places is small enough to just go with overriding.
@Poolitzer do you have an opinion on this?

PS: JobQueue fails are obviously unrelated. Please apply this fix in your PR.

@zeshuaro
Copy link
Contributor Author

zeshuaro commented Jan 6, 2021

How about deep copying the handlers and with run_async set to True? Would that be easier and also solves the problem of users passing the same handlers in different places?

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Jan 7, 2021

How about deep copying the handlers and with run_async set to True? Would that be easier and also solves the problem of users passing the same handlers in different places?

Mh, that's risky. What do we do if someone uses a custom handler with a Lock() in it? that can't be deepcopied.

@zeshuaro
Copy link
Contributor Author

zeshuaro commented Jan 7, 2021 •
edited
Loading

How about deep copying the handlers and with run_async set to True? Would that be easier and also solves the problem of users passing the same handlers in different places?

Mh, that's risky. What do we do if someone uses a custom handler with a Lock() in it? that can't be deepcopied.

Right, didn't know we can define a custom handler but that makes sense.

Going back to what we have implemented in this PR. If a user sets run_async=True in the ConversationHandler, then they will expect all handlers to be configured with run_async=True, i.e. these handlers should work and run with run_async=True. And therefore can we assume if the same handler instance is used elsewhere, it will run with run_async=True as well?

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Jan 7, 2021

Going back to what we have implemented in this PR. If a user sets run_async=True in the ConversationHandler, then they will expect all handlers to be configured with run_async=True, i.e. these handlers should work and run with run_async=True. And therefore can we assume if the same handler instance is used elsewhere, it will run with run_async=True as well?

Yes, my point is that that's rather unexpected. With proper documentation I think I'm okay with it but that's why I asked for pools opinion.

BTW, before I forget: we started adding .. versionadded:: 13.2 thingies, please add that to the new argument ;)

@Poolitzer
Copy link
Member

Poolitzer commented Jan 9, 2021

@Poolitzer do you have an opinion on this?

I do not. what would be the upside to your suggested solution?

Poolitzer
Poolitzer requested changes Jan 9, 2021
View reviewed changes
Copy link
Member

@Poolitzer Poolitzer left a comment

Choose a reason for hiding this comment

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

docs nit-picking

zeshuaro and others added 2 commits January 10, 2021 01:23
@zeshuaro @Poolitzer
Update docstring
574a5a1 
Co-authored-by: Poolitzer <25934244+Poolitzer@users.noreply.github.com>
@Bibo-Joshi
Doc nitpicking
8e41f57
@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Jan 9, 2021

I pushed a little bit more doc nitpicking … Thank you for your contribution @zeshuaro ! Merging.

@Bibo-Joshi Bibo-Joshi merged commit 0c99152 into python-telegram-bot:master Jan 9, 2021
Bibo-Joshi added a commit that referenced this pull request Jan 10, 2021
@harshil21 @Bibo-Joshi
@zeshuaro @Poolitzer
Doc fix for messageentity (#2311)
dc46913 
* Add New Shortcuts to Chat (#2291)

* Add shortcuts

* Add a note

* Add run_async Parameter to ConversationHandler (#2292)

* Add run_async parameter

* Update docstring

* Update test to explicitly specify parameter

* Fix test job queue

* Add version added tag to docs

* Update docstring

Co-authored-by: Poolitzer <25934244+Poolitzer@users.noreply.github.com>

* Doc nitpicking

Co-authored-by: Poolitzer <25934244+Poolitzer@users.noreply.github.com>
Co-authored-by: Hinrich Mahler <hinrich.mahler@freenet.de>

* Fix rendering in messageentity

Co-authored-by: Bibo-Joshi <hinrich.mahler@freenet.de>
Co-authored-by: zeshuaro <joshuaystang@gmail.com>
Co-authored-by: Poolitzer <25934244+Poolitzer@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators Jan 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Bibo-Joshi Bibo-Joshi Bibo-Joshi requested changes

@Poolitzer Poolitzer Poolitzer requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEATURE] ConversationHandler with option to override all handlers' run async option

3 participants

@zeshuaro @Bibo-Joshi @Poolitzer