Update Filters, CommandHandler and MessageHandler by Eldinnie · Pull Request #1117 · python-telegram-bot/python-telegram-bot

Eldinnie · 2018-05-22T23:36:27Z

update_filter attribute on filters
add update_type filter
Messagehandler rework
- remove allow_edited (deprecated for a while)
- set deprecated defaults to None
- Raise deprecation warning when they're used
- add sensible defaults for filters.
- rework tests
Commandhandler rework
- deprecate allow_edited
- Raise deprecation warning when used
- default to allowing edited

Makes it possible to have filters work on an update instead of message, while keeping behavior for current filters

- remove allow_edited (deprecated for a while) - set deprecated defaults to None - Raise deprecation warning when they're used - add sensible defaults for filters. - rework tests

jsmnbom

Mostly looks good! :D Added a few comments as usual :)
We gotta remember to update the wiki page too

jsmnbom · 2018-05-23T08:56:29Z

telegram/ext/commandhandler.py

    which is the text following the command split on single or consecutive whitespace characters.

+    By default the handler listens to messages as well as edited messages. To change this behavior
+    use ``~Filters.update_type.edited_message``.


add "[...] as the filter argument" maybe?

Oh and this should probably be on the messagehandler too?

jsmnbom · 2018-05-23T08:59:08Z

telegram/ext/commandhandler.py

            operators (& for and, | for or, ~ for not).
        allow_edited (:obj:`bool`, optional): Determines whether the handler should also accept
            edited messages. Default is ``False``.
+            DEPRECATED: Edited is allowed by default. To change this behavior use


The question is weather edited should be allowed by default, cause I honestly don't think so. If I'm making a bot, I'd probably make it reply to messages it receives, right? Well if it does that with an edited_message, then the bot has sent two messages (one outdated, and the new one in reply to the edited one)..
If we wanna have this be the default, we should at least provide a reply_or_edit or similar convenience method.

Hmm, maybe it;s personal, but I hate bots that don't respond to me fixing my type in a message and it doesn't respond like it should. IMO handling edited message is a sane default.

I don;t really understand what you mean with a reply_or_edit method.

I agree with the new behaviour of allowing edited messages by default.

jsmnbom · 2018-05-23T08:59:57Z

telegram/ext/commandhandler.py

-        self.filters = filters
-        self.allow_edited = allow_edited
+        if filters:
+            self.filters = Filters.update_type.messages & filters


self.filters = Non-existent at this point.

jsmnbom · 2018-05-23T09:01:30Z

telegram/ext/commandhandler.py


-        self.filters = filters
-        self.allow_edited = allow_edited
+        if filters:


Should we test that self.filters is a subclass of BaseFilter? Otherwise it will give an obscure crash when we try to & it.
Also, wouldn't this actually break simple function filters? Or have we not supported those for a while?

we never checked before, we could introduce the check here but just let them get an error if they make a mistake imo.. function filters are removed in this version anyway

If we add validation on the type of the filters argument, we should do it with other handlers as well. We can do it with another PR if we find it necessary.

jsmnbom · 2018-05-23T09:02:49Z

telegram/ext/commandhandler.py


        super(PrefixHandler, self).__init__(
-            'nocommand', callback, filters=filters, allow_edited=allow_edited, pass_args=pass_args,
+            'nocommand', callback, filters=filters, allow_edited=None, pass_args=pass_args,


Could we change 'nocommand' to None, to make it more clear that it's simply not used?
This of course requires changing a few things in the CommandHandler

That would mean changing CommandHandlers logic to accomodate this. This seemed pretty clean to me.

I think (second thought?) that PrefixHandler is a redundant code. a RegexHandler with ^prefix is so much cleaner without so much boilerplate.

jsmnbom · 2018-05-23T09:04:41Z

telegram/ext/filters.py

+        def filter(self, update):
+            return self.messages(update) or self.channel_posts(update)
+
+    update_type = _UpdateType()


Should we name it updates instead? It's simply shorter and in my opinion as descriptive

yes aggreed

- rename update_types -> updates - added some clarification to docstrings

Eldinnie · 2018-05-27T14:03:29Z

@bomjacob I changed some things accoring to your review, left some comments on others.
The wiki is already up to date with these changes

tsnoam

See comments on the code

tsnoam · 2018-05-28T19:43:00Z

telegram/ext/commandhandler.py

            operators (& for and, | for or, ~ for not).
        allow_edited (:obj:`bool`, optional): Determines whether the handler should also accept
            edited messages. Default is ``False``.
+            DEPRECATED: Edited is allowed by default. To change this behavior use


I agree with the new behaviour of allowing edited messages by default.

tsnoam · 2018-05-28T19:43:41Z

telegram/ext/commandhandler.py


-        self.filters = filters
-        self.allow_edited = allow_edited
+        if filters:


If we add validation on the type of the filters argument, we should do it with other handlers as well. We can do it with another PR if we find it necessary.

tsnoam · 2018-05-28T19:45:02Z

telegram/ext/commandhandler.py

        """
-        if (isinstance(update, Update) and
-                (update.message or update.edited_message and self.allow_edited)):
+        if (isinstance(update, Update) and update.effective_message):


nitpick: there are redundant '()'
(they were good before for elegantly handling line breaks)

nitpick2: You can negate the if clause for an early return. Then the indentation of the rest of the method will be easier on the eye.

tsnoam · 2018-05-28T19:47:36Z

telegram/ext/commandhandler.py

@@ -145,8 +163,7 @@ def check_update(self, update):
            :obj:`bool`


The return object is not bool any longer.

tsnoam · 2018-05-28T19:57:21Z

telegram/ext/commandhandler.py


        super(PrefixHandler, self).__init__(
-            'nocommand', callback, filters=filters, allow_edited=allow_edited, pass_args=pass_args,
+            'nocommand', callback, filters=filters, allow_edited=None, pass_args=pass_args,


I think (second thought?) that PrefixHandler is a redundant code. a RegexHandler with ^prefix is so much cleaner without so much boilerplate.

tsnoam · 2018-05-28T20:41:03Z

telegram/ext/filters.py

        name = 'Filters.text'

        def filter(self, message):
+            print('text_filter_filter {}'.format(repr(self)))


why do you print here?
also nitpicking: 'text_filter_filter {:r}'.format(self) is more idiomatic

Eldinnie added 4 commits May 22, 2018 22:28

update_filter attribute on filters

c5d5680

Makes it possible to have filters work on an update instead of message, while keeping behavior for current filters

add update_type filter

a9c166d

Messagehandler rework

bb54b19

- remove allow_edited (deprecated for a while) - set deprecated defaults to None - Raise deprecation warning when they're used - add sensible defaults for filters. - rework tests

Commandhandler rework

5220aef

Eldinnie requested review from jsmnbom and tsnoam May 22, 2018 23:36

Remove deprecation test from new handler

a5f8412

jsmnbom requested changes May 23, 2018

View reviewed changes

Some tweaks per CR

391fb03

- rename update_types -> updates - added some clarification to docstrings

run webhook set test only on 3.6 on appveyor

d3cb62a

tsnoam requested changes May 28, 2018

View reviewed changes

Eldinnie changed the base branch from master to V11 May 31, 2018 12:24

Eldinnie closed this Sep 21, 2018

jsmnbom changed the base branch from V11 to V12 September 21, 2018 12:46

jsmnbom reopened this Sep 21, 2018

Eldinnie mentioned this pull request Sep 21, 2018

Update Filters, CommandHandler and MessageHandler #1221

Merged

jsmnbom closed this Sep 21, 2018

github-actions bot locked and limited conversation to collaborators Aug 21, 2020

		@@ -145,8 +163,7 @@ def check_update(self, update):
		:obj:`bool`

Conversation

Eldinnie commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsmnbom left a comment

Choose a reason for hiding this comment

Uh oh!

jsmnbom May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eldinnie commented May 27, 2018

Uh oh!

tsnoam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Eldinnie commented May 22, 2018 •

edited

Loading

jsmnbom May 23, 2018 •

edited

Loading