Repository: python-telegram-bot/python-telegram-bot
Pull request: Check commands in commandhandler for args
<!--
Hey! You're PRing? Cool!
Please be sure to check out our contribution guide (https://github.com/python-telegram-bot/python-telegram-bot/blob/master/.github/CONTRIBUTING.rst).
Especially, please have a look at the check list for PRs (https://github.com/python-telegram-bot/python-telegram-bot/blob/master/.github/CONTRIBUTING.rst#checklist-for-prs). Feel free to copy (parts of) the checklist to the PR description to remind you or the maintainers of open points or if you have questions on anything.
-->
Hi, so sorry, i now realize https://github.com/python-telegram-bot/python-telegram-bot/pull/3812 is closed, but i have finished amending changes in this new PR here. So sorry for the delay 🙏 this should resolve the comments in the PR previously. 

closes #3798, supersedes #3812

Problem statement:
[FEATURE] Add args present argument to command filter 
### What kind of feature are you missing? Where do you notice a shortcoming of PTB?

There is currently no filter defined by us which simply checks if a command message has args, e.g. `start payload`

### Describe the solution you'd like

Add one argument to the command filter called `has_args` which checks if something comes after the command in the message.

### Describe alternatives you've considered

One can do this filter by themselves, but providing one would be nice

### Additional context

We could also start discussing if we want to provide some way to check the content of args, like a pattern match.

Hints from issue discussion:
Few more thoughts:

- has_args only makes sense if only_start is True
- really elaborate arg parsing is a bit out if scope imo. Something Iike argparse or click filters should rather go into ptbcontrib IMO. A midway that I see would be to allow integer input for has_args, i.e. the following mapping:
  * None: args not checked
  * True: Any number of args must be present
  * False: Args must not be present
  * Integer: The number of arguments mist match the specified number
- I'm thinking if this could be a setting of CommandHandler additionally or instead of a filter. Haven't made up my mind yet ...
hi im new to contributing to open source projects but i would like to give this a shot although i might need some hand holding 🙏
@thatguylah We are good with handholding, no worries. Feel free to join our development group at https://t.me/pythontelegrambotdev, it will probably be easier to help you there then via GitHub comments.
Ok great! @Poolitzer ill start by reading the contributing guide, do let me know if you have any references that would also come in handy, and would also appreciate if you could point me to a good place to start in the codebase as i am not very familiar with it yet. 
By now I tend to rather making this an argument of `CommandHandler`. Reasoning: `CommandHandler` is used to handle specific commands, while `MessageHandler(filters.Command(), …)` is rather used to handle arbitrary commands, e.g. for catching unknown commands. In the latter case I don't see much of a use case for parsing the arguments. @Poolitzer what's your opinion on this?
Yes I am fine with that
Hi @Poolitzer @Bibo-Joshi , to clarify does this mean that to make it an argument of `CommandHandler` will mean adding a new argument in `class CommandHandler` in `/telegram/ext/_commandhandler.py` as compared to  `class Command(MessageFilter)` in `/telegram/ext/filters.py`? 
Yes, that's what I meant :)
Naive approach outlined below, would appreciate input before i start working on my branch :)
- `class CommandHandler` already has methods `check_update` and `collect_additional_context`  both which already parse in the args after the command in some way (unless im completely mistaken here)
- naive approach would be to implement a new method within the class that simply takes in an `Update` object does similar parsing methods to `check_update` and returns a boolean if `has_args`
- It would also be good if you could provide some more context around this, eg. when would a user want to use this? 
- better (?) approach i guess would be to reuse existing code in `check_update` where possible but im not too sure either
- i guess the worse approach to take is directly modify `check_update` and return a new boolean in this method if `has_args` is true. Quite sure this would break alot of existing functionality as anything that invokes this method would have an unexpected return value (?)

Appreciate your inputs @Bibo-Joshi @Poolitzer , please correct me if im wrong on any of the above 👍 

Hi. `CommandHandler.check_update` should return `False`/`None` if the update must not be handled and it must return a tuple of (the found arguments, filter results) otherwise.
What you would have to implement is

* add an argument `has_args` to `__init__`
* save that argument as (protected) attribute
* access it in the existing `check_update` method to verify if the update should be handled or not, then return the appropriate return value

You can decide if `has_args` should only accept a boolean or if you want to implement the logic that I described [here](https://github.com/python-telegram-bot/python-telegram-bot/issues/3798#issuecomment-1634060162). As outlined in the contribution guide, we'll also need unit tests for the new behavior.

Commit message: Merge branch 'master' into check-commands-for-args

Patch to review:
diff --git a/AUTHORS.rst b/AUTHORS.rst
index 3a2395964cc..42eec2f5821 100644
--- a/AUTHORS.rst
+++ b/AUTHORS.rst
@@ -120,6 +120,7 @@ The following wonderful people contributed directly or indirectly to this projec
 - `Wagner Macedo <https://github.com/wagnerluis1982>`_
 - `wjt <https://github.com/wjt>`_
 - `Yaw Danso <https://github.com/dglitxh>`_
+- `Yao Kuan <https://github.com/thatguylah>`_
 - `zeroone2numeral2 <https://github.com/zeroone2numeral2>`_
 - `zeshuaro <https://github.com/zeshuaro>`_
 - `zpavloudis <https://github.com/zpavloudis>`_
diff --git a/telegram/ext/_commandhandler.py b/telegram/ext/_commandhandler.py
index c31673d9ba0..776e9e8c6db 100644
--- a/telegram/ext/_commandhandler.py
+++ b/telegram/ext/_commandhandler.py
@@ -88,6 +88,14 @@ async def callback(update: Update, context: CallbackContext)
             :meth:`telegram.ext.Application.process_update`. Defaults to :obj:`True`.
 
             .. seealso:: :wiki:`Concurrency`
+        has_args (:obj:`bool` | :obj:`int`, optional):
+            Determines whether the command handler should process the update or not.
+            If :obj:`True`, the handler will process any non-zero number of args.
+            If :obj:`False`, the handler will only process if there are no args.
+            if :obj:`int`, the handler will only process if there are exactly that many args.
+            Defaults to :obj:`None`, which means the handler will process any or no args.
+
+            .. versionadded:: NEXT.VERSION
 
     Raises:
         :exc:`ValueError`: When the command is too long or has illegal chars.
@@ -100,9 +108,14 @@ async def callback(update: Update, context: CallbackContext)
         block (:obj:`bool`): Determines whether the return value of the callback should be
             awaited before processing the next handler in
             :meth:`telegram.ext.Application.process_update`.
+        has_args (:obj:`bool` | :obj:`int` | None):
+            Optional argument, otherwise all implementations of :class:`CommandHandler` will break.
+            Defaults to :obj:`None`, which means the handler will process any args or no args.
+
+            .. versionadded:: NEXT.VERSION
     """
 
-    __slots__ = ("commands", "filters")
+    __slots__ = ("commands", "filters", "has_args")
 
     def __init__(
         self,
@@ -110,6 +123,7 @@ def __init__(
         callback: HandlerCallback[Update, CCT, RT],
         filters: Optional[filters_module.BaseFilter] = None,
         block: DVType[bool] = DEFAULT_TRUE,
+        has_args: Union[bool, None, int] = None,
     ):
         super().__init__(callback, block=block)
 
@@ -126,6 +140,33 @@ def __init__(
             filters if filters is not None else filters_module.UpdateType.MESSAGES
         )
 
+        self.has_args: Union[bool, None, int] = has_args
+
+    def _check_correct_args(self, args: List[str]) -> Optional[bool]:
+        """Determines whether the args are correct for this handler. Implemented in check_update().
+        Args:
+            args (:obj:`list`): The args for the handler.
+        Returns:
+            :obj:`bool`: Whether the args are valid for this handler.
+        """
+
+        has_args_none = self.has_args is None
+        has_args_true_args_present = self.has_args is True and args
+        has_args_false_args_absent = self.has_args is False and not args
+        has_args_num = isinstance(self.has_args, int) and len(args) == self.has_args
+        has_args_num_negative_int = isinstance(self.has_args, int) and self.has_args < 0
+
+        if has_args_num_negative_int:
+            raise ValueError("CommandHandler argument has_args cannot be a negative integer")
+        if (
+            (has_args_none)
+            or (has_args_true_args_present)
+            or (has_args_false_args_absent)
+            or (has_args_num)
+        ):
+            return True
+        return False
+
     def check_update(
         self, update: object
     ) -> Optional[Union[bool, Tuple[List[str], Optional[Union[bool, FilterDataDict]]]]]:
@@ -159,6 +200,9 @@ def check_update(
                 ):
                     return None
 
+                if not self._check_correct_args(args):
+                    return None
+
                 filter_result = self.filters.check_update(update)
                 if filter_result:
                     return args, filter_result
diff --git a/tests/ext/test_commandhandler.py b/tests/ext/test_commandhandler.py
index c925a494fde..c832ee57d17 100644
--- a/tests/ext/test_commandhandler.py
+++ b/tests/ext/test_commandhandler.py
@@ -181,7 +181,11 @@ async def test_basic(self, app, command):
 
     @pytest.mark.parametrize(
         "cmd",
-        ["way_too_longcommand1234567yes_way_toooooooLong", "ïñválídletters", "invalid #&* chars"],
+        [
+            "way_too_longcommand1234567yes_way_toooooooLong",
+            "ïñválídletters",
+            "invalid #&* chars",
+        ],
         ids=["too long", "invalid letter", "invalid characters"],
     )
     def test_invalid_commands(self, cmd):
@@ -259,3 +263,31 @@ async def test_context_multiple_regex(self, app, command):
             self.callback_regex2, filters=filters.Regex("one") & filters.Regex("two")
         )
         await self._test_context_args_or_regex(app, handler, command)
+
+    def test_command_has_args(self, bot):
+        """Test CHs with optional has_args specified."""
+        handler_true = CommandHandler(["test"], self.callback_basic, has_args=True)
+        handler_false = CommandHandler(["test"], self.callback_basic, has_args=False)
+        handler_int_one = CommandHandler(["test"], self.callback_basic, has_args=1)
+        handler_int_two = CommandHandler(["test"], self.callback_basic, has_args=2)
+        handler_int_negative = CommandHandler(["test"], self.callback_basic, has_args=-1)
+
+        assert is_match(handler_true, make_command_update("/test helloworld", bot=bot))
+        assert not is_match(handler_true, make_command_update("/test", bot=bot))
+
+        assert is_match(handler_false, make_command_update("/test", bot=bot))
+        assert not is_match(handler_false, make_command_update("/test helloworld", bot=bot))
+
+        assert is_match(handler_int_one, make_command_update("/test helloworld", bot=bot))
+        assert not is_match(handler_int_one, make_command_update("/test hello world", bot=bot))
+        assert not is_match(handler_int_one, make_command_update("/test", bot=bot))
+
+        assert is_match(handler_int_two, make_command_update("/test hello world", bot=bot))
+        assert not is_match(handler_int_two, make_command_update("/test helloworld", bot=bot))
+        assert not is_match(handler_int_two, make_command_update("/test", bot=bot))
+
+        # Test for ValueError when has_args is a negative integer
+        with pytest.raises(
+            ValueError, match="CommandHandler argument has_args cannot be a negative integer"
+        ):
+            is_match(handler_int_negative, make_command_update("/test hello world", bot=bot))


Code review comments:
diff_hunk: @@ -100,16 +108,22 @@ async def callback(update: Update, context: CallbackContext)
         block (:obj:`bool`): Determines whether the return value of the callback should be
             awaited before processing the next handler in
             :meth:`telegram.ext.Application.process_update`.
+        has_args (:obj:`bool` | :obj:`int` | None):
+            Optional argument, otherwise all implementations of :class:`CommandHandler` will break.
+            Defaults to :obj:`None`, which means the handler will process any args or no args.
+
+            .. versionadded:: NEXT.VERSION
     """
 
-    __slots__ = ("commands", "filters")
+    __slots__ = ("commands", "filters", "has_args")
 
     def __init__(
         self,
         command: SCT[str],
         callback: HandlerCallback[Update, CCT, RT],
         filters: Optional[filters_module.BaseFilter] = None,
         block: DVType[bool] = DEFAULT_TRUE,
+        has_args: Union[bool, None, int] = None,
path: telegram/ext/_commandhandler.py
text: @user1:
```suggestion
        has_args: Optional[bool, int] = None,
```

@author:
Hi, i tried implementing your suggestion, but in return i am failing precommit hook `mypy-ptb` 

<img width="520" alt="image" src="https://github.com/python-telegram-bot/python-telegram-bot/assets/107021083/eb992021-1cd2-44f1-9079-a2d9b559e513">

Suggest to either keep `Union[bool,None,int]` or change to `Optional[Union[bool, int]]` what do you think?

@user1:
oh huh, didn't know that. Ok whatever works for me then.

diff_hunk: @@ -181,7 +181,11 @@ async def test_basic(self, app, command):
 
     @pytest.mark.parametrize(
         "cmd",
-        ["way_too_longcommand1234567yes_way_toooooooLong", "ïñválídletters", "invalid #&* chars"],
+        [
+            "way_too_longcommand1234567yes_way_toooooooLong",
+            "ïñválídletters",
+            "invalid #&* chars",
+        ],
path: tests/ext/test_commandhandler.py
text: @user1:
Is this change related to the PR? If not, please revert

@author:
Reverted

diff_hunk: @@ -126,6 +140,33 @@ def __init__(
             filters if filters is not None else filters_module.UpdateType.MESSAGES
         )
 
+        self.has_args: Union[bool, None, int] = has_args
path: telegram/ext/_commandhandler.py
text: @user1:
```suggestion
        self.has_args: Optional[bool, int] = has_args
```

diff_hunk: @@ -126,6 +140,33 @@ def __init__(
             filters if filters is not None else filters_module.UpdateType.MESSAGES
         )
 
+        self.has_args: Union[bool, None, int] = has_args
+
+    def _check_correct_args(self, args: List[str]) -> Optional[bool]:
+        """Determines whether the args are correct for this handler. Implemented in check_update().
+        Args:
+            args (:obj:`list`): The args for the handler.
+        Returns:
+            :obj:`bool`: Whether the args are valid for this handler.
+        """
+
+        has_args_none = self.has_args is None
+        has_args_true_args_present = self.has_args is True and args
+        has_args_false_args_absent = self.has_args is False and not args
+        has_args_num = isinstance(self.has_args, int) and len(args) == self.has_args
+        has_args_num_negative_int = isinstance(self.has_args, int) and self.has_args < 0
+
+        if has_args_num_negative_int:
path: telegram/ext/_commandhandler.py
text: @user1:
please move this check to the `__init__` so that we get the exception directly at instantiation instead of later at runtime.

@author:
Done.

diff_hunk: @@ -126,6 +140,33 @@ def __init__(
             filters if filters is not None else filters_module.UpdateType.MESSAGES
         )
 
+        self.has_args: Union[bool, None, int] = has_args
+
+    def _check_correct_args(self, args: List[str]) -> Optional[bool]:
+        """Determines whether the args are correct for this handler. Implemented in check_update().
+        Args:
+            args (:obj:`list`): The args for the handler.
+        Returns:
+            :obj:`bool`: Whether the args are valid for this handler.
+        """
+
+        has_args_none = self.has_args is None
+        has_args_true_args_present = self.has_args is True and args
+        has_args_false_args_absent = self.has_args is False and not args
+        has_args_num = isinstance(self.has_args, int) and len(args) == self.has_args
+        has_args_num_negative_int = isinstance(self.has_args, int) and self.has_args < 0
+
+        if has_args_num_negative_int:
+            raise ValueError("CommandHandler argument has_args cannot be a negative integer")
+        if (
+            (has_args_none)
path: telegram/ext/_commandhandler.py
text: @user1:
IMO you can just place the conditions in here directly , i.e.

```python
if (
    (self.has_args is None)
    or (…)
    ...
):
```
That way we also make use of pythons short-circut logic, i.e. if the first condition is already `True`, then the other ones don't have to be evaluated.

@author:
Done.