Skip to content

[DISCUSSION] Handling of inits #2556

Closed
Closed
@Bibo-Joshi

Description

@Bibo-Joshi

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

  1. Make all of Updater, Dispatcher and Bot only accept what they actually need. So no changes for Dispatcher and Bot, bot Updater would change to def __init__(self, dispatcher):
  2. Make update_queue optional for Dispatcher and have it be build on init as self.update_queue = update_queue or Queue()
  3. Make job_queue such that by default a new JobQueue will be created, i.e. it will be opt-out instead of opt-in. For people who don't want to use threading & the JobQueue, this shouldn't be too bad, b/c the JQ only starts when we call Dispatcher.start. If they still want it to be None, they can pass None.
  4. 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 values
    • Dispatcher.build_from_token(TOKEN) builds a bot with the default values and then calls DP.build_from_bot
    • Updater.build_from_token/bot just calls Dispatcher.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).

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions