Feature #3798: Check commands for args #3812
Conversation
|
Also, do you perhaps think instead of returning |
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.
| 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. |
There was a problem hiding this comment.
| 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 |
| 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. |
There was a problem hiding this comment.
| 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 |
| 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 | ||
| ) |
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| _valid = self._check_correct_args(args) | ||
| if _valid is False: | ||
| return None |
There was a problem hiding this comment.
| _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)) |
There was a problem hiding this comment.
| 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 |
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
|
Closing due to inactivity |
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.☺️