What I saw from my little dive into your changes is that this is a good solutions which tackels all the problems we had, everything is covered. If the other maintainer agree, we can built the other default ideas we had (timeout, disable web preview mainly) on top of this. Lets use #1527 to track this if we come so far. Good job though, I really like this :)

Since this is such a big change and I bet that there will be a lot of discussion around this, I feel like we should give it it's own version, so I added it to the 12.3 milestone.

Hey,

I was wondering if it was possible to move this check:

https://github.com/python-telegram-bot/python-telegram-bot/pull/1490/files#diff-3b086505b88889eca9352d47789b348dL101-L102

from all the different Input classes into this check:

https://github.com/python-telegram-bot/python-telegram-bot/pull/1490/files#diff-56fa4ffcaf1325dae7804deba7d447afR1467-R1473

so that we wouldn't have as much code duplication.

Regarding the Bot class, i had the idea of using a factory function to make the defaults a bit nicer.

Assuming we want to be able to have defaults for other parameters, I suggest a Defaults class like this:

class Defaults():
    def __init__(parse_mode=None, disable_notification=False):
        self.parse_mode = parse_mode
        self.disable_notification = disable_notification

Then, we can use it like this:

def Bot(token, base_url=None, base_file_url=None, request=None, private_key=None,
        private_key_password=None, defaults=Defaults()):
    class Bot(TelegramObject):
        [...]

        @log
        def send_message(self,
                        chat_id,
                        text,
                        parse_mode=defaults.parse_mode,
                        disable_web_page_preview=None,
                        disable_notification=defaults.disable_notification,
                        reply_to_message_id=None,
                        reply_markup=None,
                        timeout=None,
                        **kwargs):

        [...]

    return Bot(token, base_url, base_file_url, request, private_key, private_key_password)

which means you can create a bot like Bot(..., defaults=Defaults(parse_mode='html')) and don't need any extra checks in all the api methods.

Now, i realize this seems a little janky with the factory and all, but i still wanted to put it out there as an idea.

Your idea for a Defaults class looks interesting, @jh0ker, especially since it makes it easy to add more defaults.

My main concern is usability. To me it seems a bit harder to understand

def Bot(token, base_url=None, base_file_url=None, request=None, private_key=None,
        private_key_password=None, defaults=Defaults(parse_mode=MARKDOWN,disable_notification=True))

than

def Bot(token, base_url=None, base_file_url=None, request=None, private_key=None,
        private_key_password=None, default_parse_mode=MARKDOWN, default_disable_notification=True)

Still, I guess I'd put maintainability before usability 😄
How do the other @python-telegram-bot/developers fell about that?

Here's a solution that does the best of both worlds (maybe?)

import functools
import inspect


class Defaults:
    parse_mode = None
    disable_notification = False

class Bot:
    def __new__(cls, *args, **kwargs):
        # Transform default_x=y kwargs into Defaults.x=y
        defaults = Defaults()
        for kwarg in list(kwargs.keys()):
            if kwarg.startswith('default_'):
                defaults.__dict__[kwarg[8:]] = kwargs[kwarg]
                del kwargs[kwarg]

        # Make an instance of the class
        instance = super(Bot, cls).__new__(cls, *args, **kwargs)
        # For each method
        for method_name, method in inspect.getmembers(instance, predicate=inspect.ismethod):
            # Get kwargs
            argspec = inspect.getfullargspec(method)
            kwarg_names = argspec.args[-len(argspec.defaults or []):]
            # Check if Defaults has a attribute that matches the kwarg name
            needs_default = [kwarg_name for kwarg_name in kwarg_names if kwarg_name in defaults.__dict__.keys()]
            # Make a dict of kwarg name and the default value
            default_kwargs = {kwarg_name: defaults.__dict__[kwarg_name] for kwarg_name in needs_default}
            # Apply the defaults using a partial
            if default_kwargs:
                setattr(instance, method_name, functools.partial(method, **default_kwargs))
        return instance

    def __init__(self, *args, **kwargs):
        pass

    def test(self, parse_mode=None, disable_notification=False):
        print('parse_mode=', parse_mode, ' ', 'disable_notification=', disable_notification)

print('bot1')  # bot1
bot1 = Bot()
bot1.test()  # parse_mode= None   disable_notification= False
bot1.test(parse_mode='md')  # parse_mode= md   disable_notification= False
bot1.test(parse_mode='html')  # parse_mode= html   disable_notification= False
bot1.test(parse_mode=None)  # parse_mode= None   disable_notification= False
bot1.test(disable_notification=False)  # parse_mode= None   disable_notification= False
bot1.test(disable_notification=None)  # parse_mode= None   disable_notification= None
bot1.test(disable_notification=True)  # parse_mode= None   disable_notification= True

print('bot2')  # bot2
bot2 = Bot(default_parse_mode='md')
bot2.test()  # parse_mode= md   disable_notification= False
bot2.test(parse_mode='md')  # parse_mode= md   disable_notification= False
bot2.test(parse_mode='html')  # parse_mode= html   disable_notification= False
bot2.test(parse_mode=None)  # parse_mode= None   disable_notification= False
bot2.test(disable_notification=False)  # parse_mode= md   disable_notification= False
bot2.test(disable_notification=None)  # parse_mode= md   disable_notification= None
bot2.test(disable_notification=True)  # parse_mode= md   disable_notification= True

@jsmnbom Seems equally janky, if not more so, but it's really cool ^^
Probably still better than the factory cause we still have the one class so we don't break isinstance checks and similar.

Your approach looks good, @jsmnbom, but it only seems to work for bot methods that get the parse_mode (or other defaultable) argument directly. Most notably, I don't see how your solution would handle answer_inline_query:

default_parse_mode is not passed to answer_inline_query as parse_mode, since it has no such parameter. But say we modify your solution to pass default_parse_mode to all methods as kwarg. Then we still have the problem, that we can't defferentiate between

InlineQueryResultArticle(
            ...,
            input_message_content=InputTextMessageContent(
                `some text',
                parse_mode=None))

and

InlineQueryResultArticle(
            ...,
            input_message_content=InputTextMessageContent(
                `some text'))

within answer_inline_query.

Or am I missing something?

@Bibo-Joshi We could still save the Defaults object in the Bot for that, couldn't we?

Simply storing default_parse_mode as attribute for Bot (or as attribute for Bot.defaults, if you will) wouldn't suffice (If I'm understanding you correctly @jh0ker). We need to make sure that the default value for parse_mode is different from None, wherever parse_mode is a parameter. That's why I came up with the DefaultValue class …

@Bibo-Joshi In my mind, there are two different problems here:

1. Setting the defaults for the methods on the Bot class
2. Setting the defaults for the Input classes passed to AnswerInlineQuery

Your PR already handles the second problem pretty well, IMO, with the help of the DefaultValue class you introduced, and the one additional check for this special value inside Bot.answer_inline_query.
But it would be nice to have a similarly neat solution for the Bot class as well, where the code isn't spread out/duplicated as much as it is right now.

Is there a third case or am i missing some other problem with this?

Yea in my opinion the best way to do it is using the snippet i posted, but then keeping a lot of your code for the InlineQuery stuff ^^

Okidok, then we're all on the same page :) Just wanted to clarify, before starting to work on it ;)
I'll go ahead and implement the changes.

mh … I implemented the changes @jsmnbom suggested for the default parameters, but now I'm having trouble adjusting the tests. Since I can't just alter bot.defaults.parse_mode, I do something like

setattr(bot, 'send_animation', functools.partial(bot.send_animation, **{'parse_mode': 'Markdown'}))

in the routines (and reset to 'None' afterwards). However that causes (at least i thinks that's the connection) other tests to fail. E.g. after test_animation.py fiddles with the bot, test_instance_method_send_animation in test_user.py will fail. As far as I can see this is because the mokeypatch.setattr('telegram.Bot.send_animation', test) doesn't work and user.send_animation still calls the actual method instead of the provided dummy.

Do you have any hint on how to handle this, @jh0ker @jsmnbom ?

Update:
I figured out that I can use

monkeypatch.setattr('telegram.Bot.send_animation, functools.partial(bot.send_animation, **{'parse_mode': 'Markdown'}))

instead of above snipped and that seems to work 💃
Will update the PR tomorrow :)

telegram.Message should get a default value for quote. This can't be handled by the bot class, but we could just define a __new__ for Message as we did for Bot. on my todo list.

Well, a __new__ method won't help at all. But adding an attribute to Message and passing the default_quote argument to Message.de_json (through all the classes the call the method, being WebhookServer, CallbackQuery, Chat, Bot and Update) should work.

Latest commits add default_disable_web_page_preview to InputTextMessagContent and default_quote for Message.reply_text and friends.

Regarding failed tests:

• Codecov complains but iisc, only lines not changed in this PR are marked red
• Codacy complains that WebhookHandler._default_quote is set in initialize instead of __init__, but this is also done for WebhookHandler.{bot, update_queue} …
• Pytest fails on test_pin_and_unpin_message even on multiple runs. Though I have added another test that pins and unpins (test_chat_data_default_quote), I don't see how this should be connected. Also, can't reproduce locally
• test-official fails b/c API 4.5 is not yet merged