Repository: python-telegram-bot/python-telegram-bot
Pull request: Improve signature of shortcuts
First POC for #2238. It's probably good to discuss it first, before applying the changes to all the other shortcuts.

Some notes:
* As per my comment on #2238, I skipped documentation of the arguments
* manually passing all the arguments to the bot methods seemed tiresome to me and is also error prone (we would have to test passing aaaaall the args), so I came up with `get_shortcut_kwargs`, which can be used in combination with pythons built-in `locals()` method. It will probably have to get a argument `local_vars_to_ignore` for some of the shortcuts, but we'll see.
* put the signature test (including annotations) in `conftest.py` to reduce code duplication in the tests
* added `Message._new_quote` instead of editing `Message._quote` for now, so the test won't fail immediately …

when ready, closes #2238

Problem statement:
[ENHANCEMENT] Improve callback signature of shortcuts
We just pass kwargs and args to the underlying function call currently for maintenance easiness. This however means that type hinting/suggestions from IDEs don't live up to their full potential. One of ptb greatest feature are our shortcuts, sparing the developers a lot of time writing out user or chat ids etc. We should spend some time improving this part of our library in my opinion.

If we want to implement this feature, at least testing for it would be a requirement, so whenever we change the underlying functions or add new shortcuts, the test tells us that we have to change the type hints/docs/etc.

Hints from issue discussion:
iirc, `inspect.getfullargspec` also gives type annotation info, that should also be compared.
Also I vote for not documenting the parameters at the shortcuts, as that is probably hard to test, easy to forget and really has no added benifit. We can just link to the corresponding bot methods documentation, if we want to be helpful.

Commit message: pre-commit

Patch to review:
diff --git a/examples/rawapibot.py b/examples/rawapibot.py
index 60129fe6e10..2df3b90fdca 100644
--- a/examples/rawapibot.py
+++ b/examples/rawapibot.py
@@ -52,7 +52,7 @@ def echo(bot: telegram.Bot) -> None:
 
         if update.message:  # your bot can receive updates without messages
             # Reply to the message
-            update.message.reply_text(update.message.text)
+            update.message.reply_text(update.message.text)  # type: ignore[arg-type]
 
 
 if __name__ == '__main__':
diff --git a/telegram/message.py b/telegram/message.py
index 38318ef821e..8c856a8ec10 100644
--- a/telegram/message.py
+++ b/telegram/message.py
@@ -21,7 +21,7 @@
 import datetime
 import sys
 from html import escape
-from typing import TYPE_CHECKING, Any, Dict, List, Optional, Union, ClassVar
+from typing import TYPE_CHECKING, Any, Dict, List, Optional, Union, ClassVar, Tuple
 
 from telegram import (
     Animation,
@@ -48,8 +48,13 @@
     VideoNote,
     Voice,
     ProximityAlertTriggered,
+    ReplyMarkup,
+)
+from telegram.utils.helpers import (
+    escape_markdown,
+    from_timestamp,
+    to_timestamp,
 )
-from telegram.utils.helpers import escape_markdown, from_timestamp, to_timestamp
 from telegram.utils.types import JSONDict
 
 if TYPE_CHECKING:
@@ -584,11 +589,47 @@ def _quote(self, kwargs: JSONDict) -> None:
             if (default_quote is None and self.chat.type != Chat.PRIVATE) or default_quote:
                 kwargs['reply_to_message_id'] = self.message_id
 
-    def reply_text(self, *args: Any, **kwargs: Any) -> 'Message':
+    def _new_quote(
+        self, quote: Optional[bool], reply_to_message_id: Optional[Union[int, str]]
+    ) -> Optional[Union[int, str]]:
+        """Modify kwargs for replying with or without quoting."""
+        if reply_to_message_id is not None:
+            return reply_to_message_id
+
+        if quote is not None:
+            if quote:
+                return self.message_id
+
+        else:
+            if self.bot.defaults:
+                default_quote = self.bot.defaults.quote
+            else:
+                default_quote = None
+            if (default_quote is None and self.chat.type != Chat.PRIVATE) or default_quote:
+                return self.message_id
+
+        return None
+
+    def reply_text(
+        self,
+        text: str,
+        parse_mode: str = None,
+        disable_web_page_preview: bool = None,
+        disable_notification: bool = False,
+        reply_to_message_id: Union[int, str] = None,
+        reply_markup: ReplyMarkup = None,
+        timeout: float = None,
+        api_kwargs: JSONDict = None,
+        allow_sending_without_reply: bool = None,
+        entities: Union[List[MessageEntity], Tuple[MessageEntity, ...]] = None,
+        quote: bool = None,
+    ) -> Optional['Message']:
         """Shortcut for::
 
             bot.send_message(update.message.chat_id, *args, **kwargs)
 
+        For the documentation of the arguments, please see :meth:`telegram.Bot.send_message`.
+
         Keyword Args:
             quote (:obj:`bool`, optional): If set to :obj:`True`, the message is sent as an actual
                 reply to this message. If ``reply_to_message_id`` is passed in ``kwargs``, this
@@ -599,8 +640,20 @@ def reply_text(self, *args: Any, **kwargs: Any) -> 'Message':
             :class:`telegram.Message`: On success, instance representing the message posted.
 
         """
-        self._quote(kwargs)
-        return self.bot.send_message(self.chat_id, *args, **kwargs)
+        reply_to_message_id = self._new_quote(quote, reply_to_message_id)
+        return self.bot.send_message(
+            chat_id=self.chat_id,
+            text=text,
+            parse_mode=parse_mode,
+            disable_web_page_preview=disable_web_page_preview,
+            disable_notification=disable_notification,
+            reply_to_message_id=reply_to_message_id,
+            reply_markup=reply_markup,
+            timeout=timeout,
+            api_kwargs=api_kwargs,
+            allow_sending_without_reply=allow_sending_without_reply,
+            entities=entities,
+        )
 
     def reply_markdown(self, *args: Any, **kwargs: Any) -> 'Message':
         """Shortcut for::
diff --git a/telegram/utils/helpers.py b/telegram/utils/helpers.py
index b25ddf83c78..75f02e49bde 100644
--- a/telegram/utils/helpers.py
+++ b/telegram/utils/helpers.py
@@ -28,7 +28,18 @@
 from numbers import Number
 from pathlib import Path
 
-from typing import TYPE_CHECKING, Any, DefaultDict, Dict, Optional, Tuple, Union, Type, cast, IO
+from typing import (
+    TYPE_CHECKING,
+    Any,
+    DefaultDict,
+    Dict,
+    Optional,
+    Tuple,
+    Union,
+    Type,
+    cast,
+    IO,
+)
 
 import pytz  # pylint: disable=E0401
 
diff --git a/tests/conftest.py b/tests/conftest.py
index aa19898666b..6c39c09f76c 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -17,12 +17,14 @@
 # You should have received a copy of the GNU Lesser Public License
 # along with this program.  If not, see [http://www.gnu.org/licenses/].
 import datetime
+import inspect
 import os
 import re
 from collections import defaultdict
 from queue import Queue
 from threading import Thread, Event
 from time import sleep
+from typing import Callable, List, Dict, Any
 
 import pytest
 import pytz
@@ -339,3 +341,68 @@ def expect_bad_request(func, message, reason):
             pytest.xfail(f'{reason}. {e}')
         else:
             raise e
+
+
+def check_shortcut_signature(
+    shortcut: Callable,
+    bot_method: Callable,
+    shortcut_kwargs: List[str],
+    additional_kwargs: List[str],
+) -> bool:
+    """
+    Checks that the signature of a shortcut matches the signature of the underlying bot method.
+
+    Args:
+        shortcut: The shortcut, e.g. :meth:`telegram.Message.reply_text`
+        bot_method: The bot method, e.g. :meth:`telegram.Bot.send_message`
+        shortcut_kwargs: The kwargs passed by the shortcut directly, e.g. ``chat_id``
+        additional_kwargs: Additional kwargs of the shortcut that the bot method doesn't have, e.g.
+            ``quote``.
+
+    Returns:
+        :obj:`bool`: Whether or not the signature matches.
+    """
+    shortcut_arg_spec = inspect.getfullargspec(shortcut)
+    effective_shortcut_args = set(shortcut_arg_spec.args).difference(additional_kwargs)
+    effective_shortcut_args.discard('self')
+
+    bot_arg_spec = inspect.getfullargspec(bot_method)
+    expected_args = set(bot_arg_spec.args).difference(shortcut_kwargs)
+    expected_args.discard('self')
+
+    args_check = expected_args == effective_shortcut_args
+    annotation_check = all(
+        bot_arg_spec.annotations[kwarg] == shortcut_arg_spec.annotations[kwarg]
+        for kwarg in effective_shortcut_args
+    )
+
+    return args_check and annotation_check
+
+
+def check_shortcut_call(
+    kwargs: Dict[str, Any],
+    bot_method: Callable,
+) -> bool:
+    """
+    Checks that a shortcut passes all the existing arguments to the underlying bot method. Use as::
+
+        send_message = message.bot.send_message
+
+        def make_assertion(*_, **kwargs):
+            return check_shortcut_call(send_message, kwargs)
+
+        monkeypatch.setattr(message.bot, 'send_message', make_assertion)
+        assert message.reply_text('foobar')
+
+
+    Args:
+        kwargs: The kwargs passed to the bot method by the shortcut
+        bot_method: The bot method, e.g. :meth:`telegram.Bot.send_message`
+
+    Returns:
+        :obj:`bool`
+    """
+    bot_arg_spec = inspect.getfullargspec(bot_method)
+    expected_args = set(bot_arg_spec.args).difference(['self'])
+
+    return expected_args == set(kwargs.keys())
diff --git a/tests/test_message.py b/tests/test_message.py
index 25c249ba54b..1ffa06aa8be 100644
--- a/tests/test_message.py
+++ b/tests/test_message.py
@@ -46,8 +46,10 @@
     PollOption,
     ProximityAlertTriggered,
     Dice,
+    Bot,
 )
 from telegram.ext import Defaults
+from tests.conftest import check_shortcut_signature, check_shortcut_call
 from tests.test_passport import RAW_PASSPORT_DATA
 
 
@@ -635,16 +637,22 @@ def test_effective_attachment(self, message_params):
         assert message_params.effective_attachment == item
 
     def test_reply_text(self, monkeypatch, message):
-        def test(*args, **kwargs):
-            id_ = args[0] == message.chat_id
-            text = args[1] == 'test'
+        send_message = message.bot.send_message
+
+        def make_assertion(*_, **kwargs):
+            id_ = kwargs['chat_id'] == message.chat_id
+            text = kwargs['text'] == 'test'
             if kwargs.get('reply_to_message_id') is not None:
                 reply = kwargs['reply_to_message_id'] == message.message_id
             else:
                 reply = True
-            return id_ and text and reply
+            return id_ and text and reply and check_shortcut_call(kwargs, send_message)
 
-        monkeypatch.setattr(message.bot, 'send_message', test)
+        assert check_shortcut_signature(
+            Message.reply_text, Bot.send_message, ['chat_id'], ['quote']
+        )
+
+        monkeypatch.setattr(message.bot, 'send_message', make_assertion)
         assert message.reply_text('test')
         assert message.reply_text('test', quote=True)
         assert message.reply_text('test', reply_to_message_id=message.message_id, quote=True)


Code review comments:
diff_hunk: @@ -52,7 +52,7 @@ def echo(bot: telegram.Bot) -> None:
 
         if update.message:  # your bot can receive updates without messages
             # Reply to the message
-            update.message.reply_text(update.message.text)
+            update.message.reply_text(update.message.text)  # type: ignore[arg-type]
path: examples/rawapibot.py
text: @user1:
that looks kinda wrong here but I guess we can leave it in?

@author:
Mypy just complains about `message.text` being `Optional[str]` instead of `str`. The only alternative would be to cast it to str, which might be even more irritation for newbies

@user2:
Could this be better if we add an assert before this line? Just to assure mypy that there is something in `Message.text`.


```suggestion
            if update.message.text is not None:
                update.message.reply_text(update.message.text)
```

@author:
I would be surporised if MyPy parsed that correctly. But even if it does, personally I find an assertion superfluous here, as `MessageHandler` already checked that, i.e. adding a run time relevant line only for a type checker seems strange to me.

@user2:
`mypy` do honor type guards/checks within a function, as it helps narrow down `Union`s and `Optional`s (which is just a shortcut of `Union[None, ...]`) of a variable in the flow. 

`mypy` only considers the type definition passed in to the function (i.e. `Message` for `update.message`) which is a general type definition for all possible messages. which is passed from the handler. In the dispatcher-handler pattern, if we want to tell `mypy` about the optional-ness outcome of each specific handler, we need to show that in the type definition, potentially thourgh a super long generic type definition that could definitely look much more “ugly” than a simple type guard in the processor function. 

Otherwise, mypy would take the `def my_processor(update: Update, ...):` literally, as if `my_processor` could be called by any code with any valid `Update`s.

<details><summary>Example</summary>

```python
from typing import Optional, List

class Message:
    text: Optional[str]


def process_message(m: Message):
    # With a type guard
    if m.text is not None:
        print(m.text.split())
        
    # Without a type guard
    print(m.text.split())
    

def mock_dispatcher(ms: List[Message]):
    for m in ms:
        if m.text is not None:
            process_message(m)


message0 = Message()
message0.text = None

message1 = Message()
message1.text = "Hello world."

mock_dispatcher([message0, message1])

```

Gives the following error in `mypy`:

```
main.py:13: error: Item "None" of "Optional[str]" has no attribute "split"
Found 1 error in 1 file (checked 1 source file)
```

Result ran with https://mypy-play.net/?mypy=latest&python=3.9
</details>

Furthermore, looking back at this specific piece code, the `echo` function is just looping through the `get_updates()` results without any handlers, so a check on the `None`ness of `Message.text` is needed anyway.

@user2:
On a sidenote, the opening comment of this file seems to be pointing itself for a `telegram.ext` example. Should it be something else instead?

https://github.com/python-telegram-bot/python-telegram-bot/blob/80b34811ab7cfb633d703f88d2a06df6e71d3551/examples/rawapibot.py#L4-L9

@author:
> Furthermore, looking back at this specific piece code, the echo function is just looping through the get_updates() results without any handlers, so a check on the Noneness of Message.text is needed anyway.

Good catch, you're right!

> On a sidenote, the opening comment of this file seems to be pointing itself for a telegram.ext example. Should it be something else instead?

Ups, seems the search & replace went too far, when we moved `echobot2.py` -> `echobot.py` and the former `echobot.py` -> `rawapibot.py`. I'll update in the doc-fixes branch