-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Refactor MessageQueue #2139
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
Comments
After some internal discussion, We'll postpone this to v14 (or later minor releases) when we switch to asyncio. This will allow us to make the bot methods use something like PS: I thinks it's important to allow users to customize the delay logic both on a global level (see #1369) and on a per-call level (see #2265) |
I updated this Python module and got deprecation warning. Sure, I know I should fix this on my own. However, looks like examples on wiki are out of date: https://github.com/python-telegram-bot/python-telegram-bot/wiki/Avoiding-flood-limits#using-mq-with-queuedmessage-decorator. |
Whoops, I went "tl;dr" too much. I just checked if the example changed. Got it, thank you. |
closing in favor of #3018 |
TL;DR:
Known bugs are
Long version:
Using
MessageQueue
currently requires quite some manual work and there also the aformentioned bugs. Time to live up to the wikis statementSo here is what my idea for a refactoring is. I'd be happy for feedback before getting started :)
Refactoring of
MessageQueue
andDelayQueue
DQ.exc_route
: I'd like to rename toerror_handler
in accordance withDispatcher.add_error_handler
.Dispatcher
is used, it would make sense to have the execution of the promises to behave like the refactoredDipsatcher._pooled
, which also includes proper error handling. That can be easily achieved by passing theDispatcher
to either theBot
or theMessageQueue
. Not sure yet, what makes more sense …__call__
methods withprocess
or whatever. That's more explicit and easier to read in the code.MQ.add/remove_delay_queue
so that users can add custom delay queues in addition to the default & the group one. One should be able to specify a parent (defaulting to the default DQ) fornew_dq
, such that everything going throguhnew_dq
will also go through the parent.MQ.add_remove_delay_queue
sohuld also start/stop theDQ
in case we're adding them at runtime. Edit: This will in fact close Multiple message queues for a bot #1369 , as I just noticeMQ
correctly, some hints are already given in [BUG] Bot hangs when using ctrl + c and MessageQueue #2012Integration into Bot
Add a
MessageQueue
toBot
by default and rebuild all methods in the fashionsuch that
delay_queue
allows to select the delay queue the method is passed through, replacing the currentqueue
andisgroup
kwargsBot.__new__
as in discussed [BUG] Defaults not working with MessageQueue #2015 , this should resolve [BUG] Defaults not working with MessageQueue #2015Bot
, withoutDispatcher
orUpdater
, one could do something likeso users can easily opt-into
MessageQueue
withbot.mq.start/stop()
Integration with
Defaults
Add
Defaults.delay_queue
defaulting toNone
(I'm not in favor of implicitly always using theMessageQueue
)Handling context managers
I think the approach detailed in #2035 is feasible (build
InputFiles
already inPromise.__init__
). I can't think of any use case, where a context manager would be used for anything else that we'd to handle …Edit: Now I can. See #2160
Backwards compatibility
All of above changes should be doable without breaking changes.
Hower, given that
MessageQueue
is essentially still in the same quite rough state as when introduced in #537 3.5 years ago, IMHO it would be okay to removethe current usage pattern (i.e. the@queuedmessage
decorator) in the next major release (which probably will be mostly a "delete all the stuff" release anyway …)The text was updated successfully, but these errors were encountered: