Flow control ability in Dispatcher #738
Conversation
|
This seems very similar to #667 Please take a look at the error's in travis' log. |
Calling super().setUp removed, it's useless and leads to errors in Python 2.7
|
Conflict with master fixed. |
There was a problem hiding this comment.
@ihoru thank you for your contribution.
I have left some (nitpick) comments on the code.
However, I have some bigger rejects about the code:
- I am not fond of the "coding by exception" design pattern. I prefer the handlers to return an enum of the desired action (or
None, for backward compatibility). I find this design pattern to force the developer to write more complex and less fluent code which is hard to maintain.
So I am inclined to ask you to change that code to be using enums.
EDIT:
After discussing the issue with the other developers we've come to conclusion that exceptions are the proper way to handle this. So you can ignore the first bullet.
-
I suspect that this code will not work with
run_async(a certain refactoring is needed forPromise&Dispatcherto make it work, but this is regardless to your proposed change, we have other outstanding issues with it.)
N.B.: I don't think that the support forrun_asyncwill work any differently if used with exceptions or enums, but both should be checked and proven. -
Following the above, the unitests should test run_async as well.
-
Documentation for the feature should be added to
Dispatcher.add_handler().
EDIT2:
5. The names Continue and Stop are basically ok, up to the limit that they should be globally exported via telegram/ext/__init__.py. So I propose other names like: DispatcherHandlerContinue & DispatcherHandlerStop.
telegram/ext/dispatcher.py
Outdated
| import weakref | ||
| from functools import wraps | ||
| from threading import Thread, Lock, Event, current_thread, BoundedSemaphore | ||
| from threading import BoundedSemaphore, Event, Lock, Thread, current_thread |
There was a problem hiding this comment.
I guess it's only an IDE error, and even though it is alphabetically sorted now - please remove it from the PR. I want the minimal possible change.
telegram/ext/dispatcher.py
Outdated
| from collections import defaultdict | ||
|
|
||
| from queue import Queue, Empty | ||
| from queue import Empty, Queue |
There was a problem hiding this comment.
Same comment as above (also, keep the empty line where it was... minimal change...)
telegram/ext/dispatcher.py
Outdated
| Args: | ||
| update (:obj:`str` | :class:`telegram.Update`): The update to process. | ||
| update (:obj:`str` | :class:`telegram.Update` | :class:`telegram.TelegramError`): The update to process. |
There was a problem hiding this comment.
We use the pep8 standard of 99 chars per line.
Please fix.
|
notice the edit on the review comments. |
|
TODO:
|
- line length fix - reverted touched imports - classes renamed Continue -> DispatcherHandlerContinue, Stop -> DispatcherHandlerStop
- Documentation of Dispatcher.add_handler improved
- global import of run_async from telegram.ext
- tests fixed
telegram/ext/dispatcher.py
Outdated
| Args: | ||
| update (:obj:`str` | :class:`telegram.Update`): The update to process. | ||
| update (:obj:`str` | :class:`telegram.Update` | :class:`telegram.TelegramError`): | ||
| The update to process. |
There was a problem hiding this comment.
Extra 4 spaces before The update to process is needed.
tests/test_dispatcher_flow.py
Outdated
| @@ -0,0 +1,135 @@ | |||
| #!/usr/bin/env python | |||
There was a problem hiding this comment.
can you please include these tests as part of test_updater.py?
|
@ihoru
|
- documentation fixes - tests moved to common file test_updater.py - @Property Promise.exception added - log warning in case of using DispatcherHandlerFlow in executed method asynchronously
|
@ihoru |
3 participants
There are situations I have very often when I need to control a flow of handlers execution.
Example for
Continue:I need to process a photo in different scopes (contexts): when a user got some state and in all other situations.
Using changes in this PR, I will create two handlers (in the same group):
First will check if a user has not a state it will raise
Continue.Example for
Stop:Some handlers have to be able to stop processing of update without any doubts if there are any other groups or not.
Look at
test_dispatcher_flow.pyif you need extra explanation.