Introduce keyword-only arguments in Bot methods

[tal66]

Hi, this closes #3016.
Made {read, write, connect, pool}_timeout , api_kwargs keyword-only arguments.
The docs generated looked fine to me, and the tests passed without much changing - did I miss anything?

(btw, happened to notice in test_message.py, that some args were in the wrong order - for example, line 347. just added argument names there)

Edit: saw the pre-commit formatting error (4 lines), will fix in the next commit after your comments

[harshil21]

Thanks for the PR! Overall looks great! Some comments:

1. imo, the quote argument of the shortcut methods in Message, Chat, User should also be keyword only since its added by PTB. But lets wait and see what the other maintainers think.

2. About tests:

   • In test_bot.py, one could add a test to check all bot methods and see if there are exactly 5 keyword arguments by checking their kind and name ({read, write, connect, pool}_timeout and api_kwargs). One could retrieve all public bot methods by using iscoroutinefunction and checking if the method doesn't start with an underscore.
   • The same would be need to done for the shortcuts, i.e in test_{chat, user, message}

3. Can you also add to the points in the .. versionchanged:: 20.0 at the the top of bot.py, chat.py, message.py, and user.py files saying that the args ({read, write...) were changed to keyword only?

[tal66]

2nd commit addressed some of your comments.

I'll add the test you asked tomorrow (UTC time).

[Bibo-Joshi]

Thanks for the nice PR! I left a new nitpicks below.

1. imo, the quote argument of the shortcut methods in Message, Chat, User should also be keyword only since its added by PTB. But lets wait and see what the other maintainers think.

I guess the need to separate that from the other arguments is not as big, but I'm okay with the idea. We should then however make all PTB-added arguments keyword-only. We have have few more of those, e.g. InlineQurey.answer(…, auto_pagination=True) and Bot.send_location(…, location=…). For the methods of Bot, those can be seen here. For the shortcut methods, that would be all the arguments that are passed check_shortcut_signature(…, addition_kwargs=[…]) in the tests, e.g. here.
Inserting a print(shortcut, additional_kwargs) here & then running the test suite once should reveal all of them.

Corresponding .. versionchanged:: 20.0 directives should be added.

2. About tests:

   • In test_bot.py, one could add a test to check all bot methods and see if there are exactly 5 keyword arguments by checking their kind and name ({read, write, connect, pool}_timeout and api_kwargs). One could retrieve all public bot methods by using iscoroutinefunction and checking if the method doesn't start with an underscore.
   • The same would be need to done for the shortcuts, i.e in test_{chat, user, message}

I think we can get away more easily if we make all ptb-added parameters kwargs-only:

• For checking the signature of bot methods, we can just check in test_official that all parameters not documented by TG are are kwargs-only.
• For checking shortcut signatures, it would suffice to ensure that the additional_kwargs are kwargs-only. all other parameters are matched against the Bot signature anyway.

@harshil21 what do you think?

@tal66 I'm aware that unit tests can be tedious to wrap your head around if you're not entirely familiar with the code base. If you have troubles with updating tests, please don't hesitate to reach out either here or in the dev-group on Telgeram!

[harshil21]

make all PTB-added arguments keyword-only

agree.

we can just check in test_official

sure, modifying that test would be easier than starting from scratch

[tal66]

Hi, for the tests I wanted to use inspect.getfullargspec, but for some reason it doesn't work as expected from inside test_official (thought of using it in check_method).

For example, print(inspect.getfullargspec(telegram.Bot.send_message)) gives FullArgSpec(args=[], varargs='args', varkw='kwargs', defaults=None, kwonlyargs=[], kwonlydefaults=None, annotations={'return': <class 'telegram._message.Message'>}) with args and kwonlyargs empty [] (even though inspect.signature does work). And it's the same for all other methods.
Any idea why?

[tal66]

well, I tried some stuff and this works: [p.name for p in inspect.signature(method).parameters.values() if p.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD]. nice. but it's still weird that getfullargspec doesn't.

[Bibo-Joshi]

Hi. getfullargspec can not "see through" decorators (at least not in all cases IIRC) and all the bot methods have a decorator for logging. inspect.signature is indeed the recommended method for inspecting the signature :)

[Bibo-Joshi]

I took the liberty to further adjust check_shortcut_signature, which also revealed one more necessary change in CallbackQuery.edit_message_caption :)
With this, I don't think there's anything left to add from my side. If @harshil21 gives a LGTM as well, we can merge :)

[tal66]

ok, thanks. i didn't run your change, but from reading it seems like an exception will be raised on the first arg that isn't a kw, and stop the for loop. whereas in my implementation it showed me the full set of args that should be kw (if it matters).

[harshil21]

LGTM! If there are some docs to updated, that can also be done later on

[Bibo-Joshi]

Well, pressing enter by accident messed up the commit message … but anyway, here we go 😄 Thanks a lot for the contribution @tal66 and thanks for the patience during the review process :)