Description
Motivation
BasePersistence
and Updater
currently accept a lot of parameters, which can become somewhat cluttered. For BP
, this could get worse after #2524.
For Updater
, we mostly allow all those arguments as convenience shortcuts so that users don't have to pass that stuff to Dispatcher
and Bot
on their own. However, I have the impression that it also leads to users not understanding the difference between the three classes Updater
, Dispatcher
and Bot
. Ofc, to just use them, you don't have to understand them in detail, but it also quickly leads to questions like "how do I send a message without getting an update"? because people don't know/remember that you can instantiate a Bot
without an Updater
.
I'd like to think about refactoring how we handle those.
BasePersistence
In this case it's probably rather easy: Instead of passing store_bot_data
, store_user_data
etc, we can just pass a tuple or - to be more user friendly - a namedtuple:
class PersistenceInput(typing.NamedTuple):
bot_data: bool = True
chat_data: bool = True
...
This can even be reused for new parameters that #2524 proposes and could be done non-breaking by allowing ways as mutually exclusive variants in a deprecation phase.
Updater
, Dispatcher
, Bot
Ultimately, I'd like to have a pattern that is convenient for simple use cases but still separaters the three classes.
First idea
Pretty simple & straight forward: instead of passing all the args one by one to Updater
, pass bot_kwargs
& dispatcher_kwargs
or namedtuples like above.
➕ Could be done non-breaking.
➖ even if you only want to pass the token, you'd have to pass bot_params=BotParams(token=token)
or so
Second idea
- Make all of
Updater
,Dispatcher
andBot
only accept what they actually need. So no changes forDispatcher
andBot
, botUpdater
would change todef __init__(self, dispatcher):
- Make
update_queue
optional forDispatcher
and have it be build on init asself.update_queue = update_queue or Queue()
- Make
job_queue
such that by default a newJobQueue
will be created, i.e. it will be opt-out instead of opt-in. For people who don't want to use threading & theJobQueue
, this shouldn't be too bad, b/c the JQ only starts when we callDispatcher.start
. If they still want it to beNone
, they can passNone
. - Add convenience setup-methods for the "minimal effort" cases. These would basically initialize everything as
Updater(TOKEN)
currently does:Dispatcher.build_from_bot(bot)
builds a dispatcher with the default valuesDispatcher.build_from_token(TOKEN)
builds a bot with the default values and then callsDP.build_from_bot
Updater.build_from_token/bot
just callsDispatcher.build_from_token/bot
.
This would convert
Updater(token, defaults=defaults, persistence=persistence)
into
bot = Bot(token, defaults=defaults)
dispatcher = Dispatcher(bot=bot, persistence=persistence)
updater = Updater(dispatcher)
Admittedly, this is a bit longer, but it also makes it more clear, which parameter is relevant for which class
Builder pattern
Noam suggested to use builder pattern like e.g.
UpdaterBuilder().build() # all defaults
UpdaterBuilder().jq(custom_jq).dp(custom_dp).build() # use custom jq and do. Use defaults for all other.
Behind the scenes it can call Updater.init() with all the values the builder chose to use.
We can add a kwarg Boolean to init which will signify it is called by the builder.
This Boolean can be used to issue a deprecation warning if builder is not used and also to go into a simpler flow of the code which will just use all the arguments received and won't try to calculate anything.
See https://t.me/c/1494805131/16075.
tbh, my first impression is that this mostly shifts the clutter from the __init__
to a number of builder methods. But at the time of writing noam didn't have a chance to reply to my comment, so take this note wtih a grain of salt 😉
edit: This could in fact be combined with the second approach.
BTW, this will also be a good chance to improve type hints a bit. E.g. the type of the bot instance could go into Updaters
/Dispatchers
/CallbackContexts
generics, so that in the callback you don't have to typing.cast(ExtBot, bot)
.