Feature #3798: Check commands for args#3812
Closed
thatguylah wants to merge 4 commits intopython-telegram-bot:masterfrom
Closed
Feature #3798: Check commands for args#3812thatguylah wants to merge 4 commits intopython-telegram-bot:masterfrom
thatguylah wants to merge 4 commits intopython-telegram-bot:masterfrom
Conversation
Contributor
Author
|
Also, do you perhaps think instead of returning |
Bibo-Joshi
requested changes
Jul 27, 2023
Member
Bibo-Joshi
left a comment
There was a problem hiding this comment.
Hi. Thakks for the PR! The overall gist of the changes look good :) I left some comments below.
Also, do you perhaps think instead of returning
Noneincheck_updatewe should raise anExceptioninstead?
No, that is not desired. This method it designed to determine whether the handler should handle the update and the return value must indicate that. Raising an exception would mean that it can not be determined whether the update should be handled.
Comment on lines
+92
to
+97
| 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. |
Member
There was a problem hiding this comment.
Suggested change
| 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. | |
| has_args (:obj:`bool` | :obj:`int`, optional): | |
| Specifies criteria on the arguments of the command that this handler should check for. | |
| If :obj:`True`, the handler will process the update if the command has any non-zero number of arguments. | |
| If :obj:`False`, the handler will only process the update if there are no arguments following the command. | |
| If a non-negative :obj:`int` is passed, the handler will only process the update if there are exactly that many arguments specified for the command. | |
| Defaults to :obj:`None`, which means the handler will not check the number of arguments. | |
| .. versionadded:: NEXT.VERSION |
Comment on lines
+110
to
+112
| 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. |
Member
There was a problem hiding this comment.
Suggested change
| 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. | |
| has_args (:obj:`bool` | :obj:`int`): | |
| Optional. Only allow commands whose arguments satisfy this criterion. See :param:`has_args` for details. | |
| .. versionadded:: NEXT.VERSION |
Comment on lines
+158
to
+169
| condition = ( | ||
| ArgsCondition.HAS_ARGS_NONE | ||
| if (self.has_args is None) | ||
| else ArgsCondition.HAS_ARGS_TRUE_ARGS_PRESENT | ||
| if (self.has_args is True and args) | ||
| # Must specify is True, otherwise if has_args is int, it will default to Truthy value. | ||
| else ArgsCondition.HAS_ARGS_FALSE_ARGS_ABSENT | ||
| if (self.has_args is False and not args) | ||
| else ArgsCondition.HAS_ARGS_NUM | ||
| if (isinstance(self.has_args, int) and len(args) == self.has_args) | ||
| else ArgsCondition.HAS_INVALID_ARGS | ||
| ) |
Member
There was a problem hiding this comment.
The enum is a nice idea, but it seems like rather heavy overkill for this use case :D especially since there are only 2 values that the enums are mapped to.
The important part are the if-clauses you specified and those look good to me. I suggest to simplify this method to something like this:
if (cond 1) or (cond 2) or (cond 3) or (cond 4):
return True
return False
Comment on lines
+206
to
+208
| _valid = self._check_correct_args(args) | ||
| if _valid is False: | ||
| return None |
Member
There was a problem hiding this comment.
Suggested change
| _valid = self._check_correct_args(args) | |
| if _valid is False: | |
| return None | |
| if not self._check_correct_args(args): | |
| return None |
| 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)) |
Member
There was a problem hiding this comment.
Suggested change
| 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 hello world", bot=bot)) | |
| assert not is_match(handler_int_one, make_command_update("/test", bot=bot)) |
same for hander_int_two :)
| filters if filters is not None else filters_module.UpdateType.MESSAGES | ||
| ) | ||
|
|
||
| self.has_args = has_args |
Member
There was a problem hiding this comment.
it would be good to check if has_args is negative and raise a ValueError if it is. We'd also need a corresponding unit test
Member
|
Closing due to inactivity |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #3798
CommandHandler allows an optional
has_argsargument that defaults toNone, but can optionally accept eitherTrue,Falseor anyint.Enumfor mapping this logic inside an internal method_check_correct_argsofclass CommandHandlercheck_updatemethod, right before sending tofiltersfor filters parsing, it_check_correct_argsand if_validthen it does nothing and passes tofilters_validis False, it early returnsNonebefore it hits filters parsing.test_commandhandler.pyforhas_argsLet me know if i need to refactor or add to more documentation that i am not aware about.☺️