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 differentInput 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 theBot 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 aDefaults class like this:

classDefaults():def__init__(parse_mode=None,disable_notification=False):self.parse_mode=parse_modeself.disable_notification=disable_notification

Then, we can use it like this:

defBot(token,base_url=None,base_file_url=None,request=None,private_key=None,private_key_password=None,defaults=Defaults()):classBot(TelegramObject):        [...]@logdefsend_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):        [...]returnBot(token,base_url,base_file_url,request,private_key,private_key_password)

which means you can create a bot likeBot(..., 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 aDefaults 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?)

importfunctoolsimportinspectclassDefaults:parse_mode=Nonedisable_notification=FalseclassBot:def__new__(cls,*args,**kwargs):# Transform default_x=y kwargs into Defaults.x=ydefaults=Defaults()forkwarginlist(kwargs.keys()):ifkwarg.startswith('default_'):defaults.__dict__[kwarg[8:]]=kwargs[kwarg]delkwargs[kwarg]# Make an instance of the classinstance=super(Bot,cls).__new__(cls,*args,**kwargs)# For each methodformethod_name,methodininspect.getmembers(instance,predicate=inspect.ismethod):# Get kwargsargspec=inspect.getfullargspec(method)kwarg_names=argspec.args[-len(argspec.defaultsor []):]# Check if Defaults has a attribute that matches the kwarg nameneeds_default= [kwarg_nameforkwarg_nameinkwarg_namesifkwarg_nameindefaults.__dict__.keys()]# Make a dict of kwarg name and the default valuedefault_kwargs= {kwarg_name:defaults.__dict__[kwarg_name]forkwarg_nameinneeds_default}# Apply the defaults using a partialifdefault_kwargs:setattr(instance,method_name,functools.partial(method,**default_kwargs))returninstancedef__init__(self,*args,**kwargs):passdeftest(self,parse_mode=None,disable_notification=False):print('parse_mode=',parse_mode,' ','disable_notification=',disable_notification)print('bot1')# bot1bot1=Bot()bot1.test()# parse_mode= None   disable_notification= Falsebot1.test(parse_mode='md')# parse_mode= md   disable_notification= Falsebot1.test(parse_mode='html')# parse_mode= html   disable_notification= Falsebot1.test(parse_mode=None)# parse_mode= None   disable_notification= Falsebot1.test(disable_notification=False)# parse_mode= None   disable_notification= Falsebot1.test(disable_notification=None)# parse_mode= None   disable_notification= Nonebot1.test(disable_notification=True)# parse_mode= None   disable_notification= Trueprint('bot2')# bot2bot2=Bot(default_parse_mode='md')bot2.test()# parse_mode= md   disable_notification= Falsebot2.test(parse_mode='md')# parse_mode= md   disable_notification= Falsebot2.test(parse_mode='html')# parse_mode= html   disable_notification= Falsebot2.test(parse_mode=None)# parse_mode= None   disable_notification= Falsebot2.test(disable_notification=False)# parse_mode= md   disable_notification= Falsebot2.test(disable_notification=None)# parse_mode= md   disable_notification= Nonebot2.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 breakisinstance checks and similar.

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

default_parse_mode is not passed toanswer_inline_query asparse_mode, since it has no such parameter. But say we modify your solution to passdefault_parse_mode toall 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'))

withinanswer_inline_query.

Or am I missing something?

@Bibo-Joshi We could still save theDefaults object in theBot for that, couldn't we?

Simply storingdefault_parse_mode as attribute forBot (or as attribute forBot.defaults, if you will) wouldn't suffice (If I'm understanding you correctly@jh0ker). We need to make sure that the default value forparse_mode is different fromNone, whereverparse_mode is a parameter. That's why I came up with theDefaultValue class …

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

1. Setting the defaults for the methods on theBot class
2. Setting the defaults for theInput classes passed toAnswerInlineQuery

Your PR already handles the second problem pretty well, IMO, with the help of theDefaultValue class you introduced, and the one additional check for this special value insideBot.answer_inline_query.
But it would be nice to have a similarly neat solution for theBot 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 alterbot.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. aftertest_animation.py fiddles with the bot,test_instance_method_send_animation intest_user.py will fail. As far as I can see this is because themokeypatch.setattr('telegram.Bot.send_animation', test) doesn't work anduser.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 forquote. This can't be handled by the bot class, but we could just define a__new__ for Message as we did forBot. on my todo list.

Well, a__new__ method won't help at all. But adding an attribute toMessage and passing thedefault_quote argument toMessage.de_json (through all the classes the call the method, beingWebhookServer,CallbackQuery,Chat,Bot andUpdate) should work.

Latest commits adddefault_disable_web_page_preview toInputTextMessagContent anddefault_quote forMessage.reply_text and friends.

Regarding failed tests:

• Codecov complains but iisc, only lines not changed in this PR are marked red
• Codacy complains thatWebhookHandler._default_quote is set ininitialize instead of__init__, but this is also done forWebhookHandler.{bot, update_queue} …
• Pytest fails ontest_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