Repository: python-telegram-bot/python-telegram-bot
Pull request: Refactor conv warnings
<!--
Hey! You're PRing? Cool! Please have a look at the below checklist. It's here to help both you and the maintainers to remember some aspects. Make sure to check out our contribution guide (https://github.com/python-telegram-bot/python-telegram-bot/blob/master/.github/CONTRIBUTING.rst).
-->

### Checklist for PRs

- [ ] Added `.. versionadded:: version`, `.. versionchanged:: version` or `.. deprecated:: version` to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
- [x] Created new or adapted existing unit tests
- [ ] Added myself alphabetically to `AUTHORS.rst` (optional)


This PR closes #2615.

I did make a decision while implementing this: I don't think it makes sense to break the for loops after one warning. The warnings menu will not show more then one unique warning anyway, and we swallow non unique ones this way or unique ones if the user changes their `filterwarning` method. 


Problem statement:
[FEATURE] Issue an warning if PollHandler (etc.?) is added into a conv handler
We warn for quite some handlers if the surrounding variables dont work out for them (CallbackQuery, ChosenInlineResultHandler). We can at least add Poll and PollAnswerHandler to it, maybe some more I missed. We can maybe even write some clearer error messages. Suggestions are welcome :)

Hints from issue discussion:
IISC correctly, we currently warn for 3 different cases:

1. nested conversations + `conversation_timeout`
2. combinations of `CallbackQueryHandler` and `per_message`
3. `InlineQueryHandler` and `ChosenInlineResultHandler`

The first two are rather reasonable IMO since there is a very close connection between the handler and the corresponding setting.
For the latter, this is basically a "The handlers won't file if the `per_chat/user` settings prevent updates with a `update.effective_chat/user` combination that the handler would need", which is also the case for `Poll` and `PollAnswerHandler`.

I suggest to either

* Extend the warning to __all__ such handlers in `telegram.ext` or
* just elaborate the note in the docs https://github.com/python-telegram-bot/python-telegram-bot/blob/1fdaaac8094c9d76c34c8c8e8c9add16080e75e7/telegram/ext/conversationhandler.py#L70-L80

Personally, I'm in favor of the latter …
I like the former (I tried to imply that with the etc. part in the title). Doc notes are good, but if a warning appears while programming, users are more likely to actually see that they are doing a mistake. And the issue with a Poll Update is that it has no identifying options for a chat (not even the per_user Chosen has), so it cant fit in a conversationhandler at all.
@Poolitzer fair enough :)
Cool, I will tackle this in the appropriate stage then!
while tackling this, I realized it might be cool if we offer the user a choice that they can enable/disable callbackquery for updates without message context, so one never has to see a warning for valid code.
Update: This is included in the FAQ now: https://git.io/JtcyU

Commit message: Feat: Add TypeHandler to check

also added a checkmark to the PR template

Patch to review:
diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md
index 6122e7fb647..91c16804b5d 100644
--- a/.github/pull_request_template.md
+++ b/.github/pull_request_template.md
@@ -28,6 +28,7 @@ Hey! You're PRing? Cool! Please have a look at the below checklist. It's here to
     - [ ] Link new and existing constants in docstrings instead of hard coded number and strings
     - [ ] Add new message types to `Message.effective_attachment`
     - [ ] Added new handlers for new update types
+      - [ ] Add the handlers to the warning loop in the `ConversationHandler`
     - [ ] Added new filters for new message (sub)types
     - [ ] Added or updated documentation for the changed class(es) and/or method(s)
     - [ ] Updated the Bot API version number in all places: `README.rst` and `README_RAW.rst` (including the badge), as well as `telegram.constants.BOT_API_VERSION`
diff --git a/telegram/ext/_conversationhandler.py b/telegram/ext/_conversationhandler.py
index 2685a95d608..50489b20a17 100644
--- a/telegram/ext/_conversationhandler.py
+++ b/telegram/ext/_conversationhandler.py
@@ -45,6 +45,9 @@
     DispatcherHandlerStop,
     Handler,
     InlineQueryHandler,
+    StringCommandHandler,
+    StringRegexHandler,
+    TypeHandler,
 )
 from telegram._utils.warnings import warn
 from telegram.ext._utils.promise import Promise
@@ -239,6 +242,14 @@ def __init__(
         map_to_parent: Dict[object, object] = None,
         run_async: bool = False,
     ):
+        # these imports need to be here because of circular import error otherwise
+        from telegram.ext import (  # pylint: disable=import-outside-toplevel
+            ShippingQueryHandler,
+            PreCheckoutQueryHandler,
+            PollHandler,
+            PollAnswerHandler,
+        )
+
         self.run_async = run_async
 
         self._entry_points = entry_points
@@ -283,35 +294,73 @@ def __init__(
         for state_handlers in states.values():
             all_handlers.extend(state_handlers)
 
-        if self.per_message:
-            for handler in all_handlers:
+        # this loop is going to warn the user about handlers which can work unexpected
+        # in conversations
+
+        # this link will be added to all warnings tied to per_* setting
+        per_faq_link = (
+            " Read this FAQ entry to learn more about the per_* settings https://git.io/JtcyU."
+        )
+
+        for handler in all_handlers:
+            if isinstance(handler, (StringCommandHandler, StringRegexHandler)):
+                warn(
+                    "The `ConversationHandler` only handles updates of type `telegram.Update`. "
+                    f"{handler.__class__.__name__} handles updates of type `str`.",
+                    stacklevel=2,
+                )
+            if isinstance(handler, TypeHandler) and handler.type is not Update:
+                warn(
+                    "The `ConversationHandler` only handles updates of type `telegram.Update`."
+                    f" The TypeHandler is set to handle {handler.type.__name__}.",
+                    stacklevel=2,
+                )
+            if isinstance(handler, PollHandler):
+                warn(
+                    "PollHandler will never trigger in a conversation since it has no information "
+                    "about the chat or the user who voted in it. Do you mean the "
+                    "`PollAnswerHandler`?",
+                    stacklevel=2,
+                )
+
+            if (
+                self.per_chat
+                and self.per_user
+                and (
+                    isinstance(
+                        handler,
+                        (
+                            ShippingQueryHandler,
+                            InlineQueryHandler,
+                            ChosenInlineResultHandler,
+                            PreCheckoutQueryHandler,
+                            PollAnswerHandler,
+                        ),
+                    )
+                )
+            ):
+                warn(
+                    f"Updates handled by {handler.__class__.__name__} only have information about "
+                    f"the user, so this handler wont ever be triggered if `per_chat=True`."
+                    + per_faq_link,
+                    stacklevel=2,
+                )
+
+            if self.per_message:
                 if not isinstance(handler, CallbackQueryHandler):
                     warn(
                         "If 'per_message=True', all entry points, state handlers, and fallbacks"
                         " must be 'CallbackQueryHandler', since no other handlers "
-                        "have a message context.",
+                        "have a message context." + per_faq_link,
                         stacklevel=2,
                     )
-                    break
-        else:
-            for handler in all_handlers:
+            else:
                 if isinstance(handler, CallbackQueryHandler):
                     warn(
                         "If 'per_message=False', 'CallbackQueryHandler' will not be "
-                        "tracked for every message.",
+                        "tracked for every message." + per_faq_link,
                         stacklevel=2,
                     )
-                    break
-
-        if self.per_chat:
-            for handler in all_handlers:
-                if isinstance(handler, (InlineQueryHandler, ChosenInlineResultHandler)):
-                    warn(
-                        "If 'per_chat=True', 'InlineQueryHandler' can not be used, "
-                        "since inline queries have no chat context.",
-                        stacklevel=2,
-                    )
-                    break
 
         if self.conversation_timeout:
             for handler in all_handlers:
@@ -322,7 +371,6 @@ def __init__(
                         "differently from what you expect.",
                         stacklevel=2,
                     )
-                    break
 
         if self.run_async:
             for handler in all_handlers:
diff --git a/tests/test_conversationhandler.py b/tests/test_conversationhandler.py
index 44edea95e71..d7d8492693a 100644
--- a/tests/test_conversationhandler.py
+++ b/tests/test_conversationhandler.py
@@ -18,6 +18,7 @@
 # along with this program.  If not, see [http://www.gnu.org/licenses/].
 import logging
 from time import sleep
+from warnings import filterwarnings
 
 import pytest
 from flaky import flaky
@@ -45,7 +46,15 @@
     DispatcherHandlerStop,
     TypeHandler,
     JobQueue,
+    StringCommandHandler,
+    StringRegexHandler,
+    PollHandler,
+    ShippingQueryHandler,
+    ChosenInlineResultHandler,
+    PreCheckoutQueryHandler,
+    PollAnswerHandler,
 )
+from telegram.warnings import PTBUserWarning
 
 
 @pytest.fixture(scope='class')
@@ -1325,112 +1334,137 @@ def slowbrew(_update, context):
         assert handler.conversations.get((self.group.id, user1.id)) is None
         assert self.is_timeout
 
-    def test_conversation_timeout_warning_only_shown_once(self, recwarn):
+    def test_handlers_generate_warning(self, recwarn):
+        # this function tests all handler + per_* setting combinations.
+
+        # the warning message action needs to be set to always,
+        # otherwise only the first occurrence will be issued
+        filterwarnings(action="always", category=PTBUserWarning)
+
+        # this class doesn't do anything, its just not the Update class
+        class NotUpdate:
+            pass
+
+        # this conversation handler has the string, string_regex, Pollhandler and TypeHandler
+        # which should all generate a warning no matter the per_* setting. TypeHandler should
+        # not when the class is Update
         ConversationHandler(
-            entry_points=self.entry_points,
+            entry_points=[StringCommandHandler("code", self.code)],
             states={
-                self.THIRSTY: [
-                    ConversationHandler(
-                        entry_points=self.entry_points,
-                        states={
-                            self.BREWING: [CommandHandler('pourCoffee', self.drink)],
-                        },
-                        fallbacks=self.fallbacks,
-                    )
-                ],
-                self.DRINKING: [
-                    ConversationHandler(
-                        entry_points=self.entry_points,
-                        states={
-                            self.CODING: [CommandHandler('startCoding', self.code)],
-                        },
-                        fallbacks=self.fallbacks,
-                    )
+                self.BREWING: [
+                    StringRegexHandler("code", self.code),
+                    PollHandler(self.code),
+                    TypeHandler(NotUpdate, self.code),
                 ],
             },
-            fallbacks=self.fallbacks,
-            conversation_timeout=100,
+            fallbacks=[TypeHandler(Update, self.code)],
         )
-        assert len(recwarn) == 1
-        assert str(recwarn[0].message) == (
-            "Using `conversation_timeout` with nested conversations is currently not "
-            "supported. You can still try to use it, but it will likely behave "
-            "differently from what you expect."
-        )
-        assert recwarn[0].filename == __file__, "incorrect stacklevel!"
 
-    def test_per_message_warning_is_only_shown_once(self, recwarn):
+        # these handlers should all raise a warning when per_chat is True
         ConversationHandler(
-            entry_points=self.entry_points,
+            entry_points=[ShippingQueryHandler(self.code)],
             states={
-                self.THIRSTY: [CommandHandler('pourCoffee', self.drink)],
-                self.BREWING: [CommandHandler('startCoding', self.code)],
+                self.BREWING: [
+                    InlineQueryHandler(self.code),
+                    PreCheckoutQueryHandler(self.code),
+                    PollAnswerHandler(self.code),
+                ],
             },
-            fallbacks=self.fallbacks,
-            per_message=True,
-        )
-        assert len(recwarn) == 1
-        assert str(recwarn[0].message) == (
-            "If 'per_message=True', all entry points, state handlers, and fallbacks"
-            " must be 'CallbackQueryHandler', since no other handlers"
-            " have a message context."
+            fallbacks=[ChosenInlineResultHandler(self.code)],
+            per_chat=True,
         )
-        assert recwarn[0].filename == __file__, "incorrect stacklevel!"
 
-    def test_per_message_but_not_per_chat_warning(self, recwarn):
+        # the CallbackQueryHandler should *not* raise when per_message is True,
+        # but any other one should
         ConversationHandler(
-            entry_points=[CallbackQueryHandler(self.code, "code")],
+            entry_points=[CallbackQueryHandler(self.code)],
             states={
-                self.BREWING: [CallbackQueryHandler(self.code, "code")],
+                self.BREWING: [CommandHandler("code", self.code)],
             },
-            fallbacks=[CallbackQueryHandler(self.code, "code")],
+            fallbacks=[CallbackQueryHandler(self.code)],
             per_message=True,
-            per_chat=False,
-        )
-        assert len(recwarn) == 1
-        assert str(recwarn[0].message) == (
-            "If 'per_message=True' is used, 'per_chat=True' should also be used, "
-            "since message IDs are not globally unique."
         )
-        assert recwarn[0].filename == __file__, "incorrect stacklevel!"
 
-    def test_per_message_false_warning_is_only_shown_once(self, recwarn):
+        # the CallbackQueryHandler should raise when per_message is False
         ConversationHandler(
-            entry_points=self.entry_points,
+            entry_points=[CommandHandler("code", self.code)],
             states={
-                self.THIRSTY: [CallbackQueryHandler(self.drink)],
-                self.BREWING: [CallbackQueryHandler(self.code)],
+                self.BREWING: [CommandHandler("code", self.code)],
             },
-            fallbacks=self.fallbacks,
+            fallbacks=[CallbackQueryHandler(self.code)],
             per_message=False,
         )
-        assert len(recwarn) == 1
+
+        # the overall handlers raising an error is 11
+        assert len(recwarn) == 11
+        # now we test the messages, they are raised in the order they are inserted
+        # into the conversation handler
         assert str(recwarn[0].message) == (
-            "If 'per_message=False', 'CallbackQueryHandler' will not be "
-            "tracked for every message."
+            "The `ConversationHandler` only handles updates of type `telegram.Update`. "
+            "StringCommandHandler handles updates of type `str`."
+        )
+        assert str(recwarn[1].message) == (
+            "The `ConversationHandler` only handles updates of type `telegram.Update`. "
+            "StringRegexHandler handles updates of type `str`."
+        )
+        assert str(recwarn[2].message) == (
+            "PollHandler will never trigger in a conversation since it has no information "
+            "about the chat or the user who voted in it. Do you mean the "
+            "`PollAnswerHandler`?"
+        )
+        assert str(recwarn[3].message) == (
+            "The `ConversationHandler` only handles updates of type `telegram.Update`. "
+            "The TypeHandler is set to handle NotUpdate."
         )
-        assert recwarn[0].filename == __file__, "incorrect stacklevel!"
 
-    def test_warnings_per_chat_is_only_shown_once(self, recwarn):
-        def hello(update, context):
-            return self.BREWING
+        per_faq_link = (
+            " Read this FAQ entry to learn more about the per_* settings https://git.io/JtcyU."
+        )
 
-        def bye(update, context):
-            return ConversationHandler.END
+        assert str(recwarn[4].message) == (
+            "Updates handled by ShippingQueryHandler only have information about the user,"
+            " so this handler wont ever be triggered if `per_chat=True`." + per_faq_link
+        )
+        assert str(recwarn[5].message) == (
+            "Updates handled by ChosenInlineResultHandler only have information about the user,"
+            " so this handler wont ever be triggered if `per_chat=True`." + per_faq_link
+        )
+        assert str(recwarn[6].message) == (
+            "Updates handled by InlineQueryHandler only have information about the user,"
+            " so this handler wont ever be triggered if `per_chat=True`." + per_faq_link
+        )
+        assert str(recwarn[7].message) == (
+            "Updates handled by PreCheckoutQueryHandler only have information about the user,"
+            " so this handler wont ever be triggered if `per_chat=True`." + per_faq_link
+        )
+        assert str(recwarn[8].message) == (
+            "Updates handled by PollAnswerHandler only have information about the user,"
+            " so this handler wont ever be triggered if `per_chat=True`." + per_faq_link
+        )
+        assert str(recwarn[9].message) == (
+            "If 'per_message=True', all entry points, state handlers, and fallbacks must be "
+            "'CallbackQueryHandler', since no other handlers have a message context."
+            + per_faq_link
+        )
+        assert str(recwarn[10].message) == (
+            "If 'per_message=False', 'CallbackQueryHandler' will not be tracked for "
+            "every message." + per_faq_link
+        )
 
+    def test_per_message_but_not_per_chat_warning(self, recwarn):
         ConversationHandler(
-            entry_points=self.entry_points,
+            entry_points=[CallbackQueryHandler(self.code, "code")],
             states={
-                self.THIRSTY: [InlineQueryHandler(hello)],
-                self.BREWING: [InlineQueryHandler(bye)],
+                self.BREWING: [CallbackQueryHandler(self.code, "code")],
             },
-            fallbacks=self.fallbacks,
-            per_chat=True,
+            fallbacks=[CallbackQueryHandler(self.code, "code")],
+            per_message=True,
+            per_chat=False,
         )
         assert len(recwarn) == 1
         assert str(recwarn[0].message) == (
-            "If 'per_chat=True', 'InlineQueryHandler' can not be used,"
-            " since inline queries have no chat context."
+            "If 'per_message=True' is used, 'per_chat=True' should also be used, "
+            "since message IDs are not globally unique."
         )
         assert recwarn[0].filename == __file__, "incorrect stacklevel!"
 


Code review comments:
diff_hunk: @@ -308,32 +309,40 @@ def __init__(
                     f"{handler.__class__.__name__} handles updates of type `str`.",
                     stacklevel=2,
                 )
+            if isinstance(handler, TypeHandler) and handler.type is not Update:
+                warn(
+                    "The `ConversationHandler` only handles updates of type `telegram.Update`."
+                    f" The TypeHandler is set to handle {handler.type.__name__}.",
+                    stacklevel=2,
+                )
             if isinstance(handler, PollHandler):
                 warn(
                     "PollHandler will never trigger in a conversation since it has no information "
                     "about the chat or the user who voted in it. Do you mean the "
                     "`PollAnswerHandler`?",
                     stacklevel=2,
                 )
-            # we can assume per_user is set. If its not, per_message has to be set, otherwise the
-            # user would have faced the ValueError that all per_* settings cant be false. If
-            # per_message is set, the user will get a warning later.
+
             if (
-                isinstance(
-                    handler,
-                    (
-                        ShippingQueryHandler,
-                        InlineQueryHandler,
-                        ChosenInlineResultHandler,
-                        PreCheckoutQueryHandler,
-                        PollAnswerHandler,
-                    ),
+                self.per_chat
+                and self.per_user
path: telegram/ext/_conversationhandler.py
text: @user1:
shouldn't the warning show independent of the `per_user` setting?

@author:
fair enough. I misinterpreted your [previous comment](https://github.com/python-telegram-bot/python-telegram-bot/pull/2755#discussion_r739639248) then. Not sure why I thought this is a good idea though

diff_hunk: @@ -283,35 +294,73 @@ def __init__(
         for state_handlers in states.values():
             all_handlers.extend(state_handlers)
 
-        if self.per_message:
-            for handler in all_handlers:
+        # this loop is going to warn the user about handlers which can work unexpected
+        # in conversations
+
+        # this link will be added to all warnings tied to per_* setting
+        per_faq_link = (
+            " Read this FAQ entry to learn more about the per_* settings https://git.io/JtcyU."
+        )
+
+        for handler in all_handlers:
+            if isinstance(handler, (StringCommandHandler, StringRegexHandler)):
+                warn(
+                    "The `ConversationHandler` only handles updates of type `telegram.Update`. "
+                    f"{handler.__class__.__name__} handles updates of type `str`.",
+                    stacklevel=2,
+                )
+            if isinstance(handler, TypeHandler) and handler.type is not Update:
path: telegram/ext/_conversationhandler.py
text: @user1:
these ifs can be converted to elifs

diff_hunk: @@ -283,35 +294,73 @@ def __init__(
         for state_handlers in states.values():
             all_handlers.extend(state_handlers)
 
-        if self.per_message:
-            for handler in all_handlers:
+        # this loop is going to warn the user about handlers which can work unexpected
+        # in conversations
+
+        # this link will be added to all warnings tied to per_* setting
+        per_faq_link = (
+            " Read this FAQ entry to learn more about the per_* settings https://git.io/JtcyU."
+        )
+
+        for handler in all_handlers:
+            if isinstance(handler, (StringCommandHandler, StringRegexHandler)):
+                warn(
+                    "The `ConversationHandler` only handles updates of type `telegram.Update`. "
+                    f"{handler.__class__.__name__} handles updates of type `str`.",
+                    stacklevel=2,
+                )
+            if isinstance(handler, TypeHandler) and handler.type is not Update:
+                warn(
+                    "The `ConversationHandler` only handles updates of type `telegram.Update`."
+                    f" The TypeHandler is set to handle {handler.type.__name__}.",
+                    stacklevel=2,
+                )
+            if isinstance(handler, PollHandler):
+                warn(
+                    "PollHandler will never trigger in a conversation since it has no information "
+                    "about the chat or the user who voted in it. Do you mean the "
+                    "`PollAnswerHandler`?",
+                    stacklevel=2,
+                )
+
+            if (
+                self.per_chat
+                and self.per_user
+                and (
+                    isinstance(
+                        handler,
+                        (
+                            ShippingQueryHandler,
+                            InlineQueryHandler,
+                            ChosenInlineResultHandler,
+                            PreCheckoutQueryHandler,
+                            PollAnswerHandler,
+                        ),
+                    )
+                )
+            ):
+                warn(
+                    f"Updates handled by {handler.__class__.__name__} only have information about "
+                    f"the user, so this handler wont ever be triggered if `per_chat=True`."
path: telegram/ext/_conversationhandler.py
text: @user1:
```suggestion
                    f"the user, so this handler won't ever be triggered if `per_chat=True`."
```

diff_hunk: @@ -283,35 +294,73 @@ def __init__(
         for state_handlers in states.values():
             all_handlers.extend(state_handlers)
 
-        if self.per_message:
-            for handler in all_handlers:
+        # this loop is going to warn the user about handlers which can work unexpected
+        # in conversations
+
+        # this link will be added to all warnings tied to per_* setting
+        per_faq_link = (
+            " Read this FAQ entry to learn more about the per_* settings https://git.io/JtcyU."
+        )
+
+        for handler in all_handlers:
+            if isinstance(handler, (StringCommandHandler, StringRegexHandler)):
+                warn(
+                    "The `ConversationHandler` only handles updates of type `telegram.Update`. "
+                    f"{handler.__class__.__name__} handles updates of type `str`.",
+                    stacklevel=2,
+                )
+            if isinstance(handler, TypeHandler) and handler.type is not Update:
path: telegram/ext/_conversationhandler.py
text: @user1:
```suggestion
            if isinstance(handler, TypeHandler) and not issubclass(handler.type, Update):
```

@user2:
Shouldn't it be `and not issubclass(..)`? 

Also that part should be added to the tests, i.e TypeHandler with a subclass of Update should not raise a warning

@author:
it is: https://github.com/python-telegram-bot/python-telegram-bot/blob/5bf1d14e39e238ba99f2a04e866428a9b0db7480/tests/test_conversationhandler.py#L1357-L1360

I edited bibos code.