harshil21 · GitHub

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.

  1. 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!

Read the original on github.com ↗