rework CommandHandler by Eldinnie · Pull Request #1114 · python-telegram-bot/python-telegram-bot

Eldinnie · 2018-05-21T13:24:52Z

Only register valid botcommands, else raise ValueError
Make CommandHandler strict
Only register valid botcommands, else raise ValueError
Add PrefixHandler

(pycharm can be an arse sometimes)

…ilters themselves

- Fixes per Eldinnie's comments. - Fixes per warnings when running sphinx.

Context Based Handlers -> Context Based Callbacks No longer use_context args on every single Handler Instead set dispatcher/updater .use_context=True to use Works with - Handler callbacks - Error handler callbacks - Job callbacks Change examples to context based callbacks so new users are not confused Rename and move the context object from Handlers.HandlerContext to CallbackContext, since it doesn't only apply to handlers anymore. Fix tests by adding a new fixture `cpd` which is a dispatcher with use_context=True

Only register valid botcommands, else raise ValueError

jsmnbom · 2018-05-21T14:40:35Z

telegram/ext/commandhandler.py

        else:
            self.command = [x.lower() for x in command]
+        for comm in self.command:
+            if not re.match(r'^[\da-z_]{1,32}$', comm):


I'm not sure that 32 chars is a hard limit, dispite the docs... Do we care?

Both my phone and my desktop app, highlights commands that are longer than 32 chars at least.

There's a discrepancy here. @Botfather won't let you add too long commands for suggestion, but they are recognised as entities.

I vote to stay with the docs and enforce 32 chars

jsmnbom · 2018-05-21T14:43:17Z

telegram/ext/commandhandler.py

-                    command = first_word[1:].split('@')
-                    command.append(
-                        message.bot.username)  # in case the command was sent without a username
+            if message.entities and message.entities[0].type == MessageEntity.BOT_COMMAND and \


Really not a fan of \ in code.. could we wrap it in parenthesis instead?

jsmnbom · 2018-05-21T14:44:47Z

telegram/ext/commandhandler.py

-                        message.bot.username)  # in case the command was sent without a username
+            if message.entities and message.entities[0].type == MessageEntity.BOT_COMMAND and \
+                    message.entities[0].offset == 0:
+                command = message.text[1:message.entities[0].length]


I know this technically works because command can only be ascii, but maybe using the parse entity method would be a good idea here

I think it's easiest and safest this way.
If we used parse_entities we would have to search the dict for either message.text.split()[0] or check every entity for it's offset. Dicts are not ordered on <py3.5.
Since as you pointed out the entity is ascii and we know for sure it should be the first one in message.entites I think this is the best way.

jsmnbom · 2018-05-21T14:48:01Z

telegram/ext/commandhandler.py

+            if message.entities and message.entities[0].type == MessageEntity.BOT_COMMAND and \
+                    message.entities[0].offset == 0:
+                command = message.text[1:message.entities[0].length]
+                args = message.text.split()[1:]


Wouldn't it be more correct to do message.text[len(command):].split() ?

This is the way we always calculated args, And the result should be the same I guess, except for an extra len() call in your proposal.

Depending on the internals of .split() and telegrams entity calculations, but yeah, you're probably right ^^

jsmnbom · 2018-05-21T14:49:33Z

telegram/ext/commandhandler.py

+    """Handler class to handle custom prefix commands
+
+    This is a intermediate handler between :class:`MessageHandler` and :class:`CommandHandler`.
+    It supports configurable commands with the same options as commandhandler. It will respond to


CommandHandler*

jsmnbom · 2018-05-21T14:50:55Z

telegram/ext/commandhandler.py

+            Filters.
+        allow_edited (:obj:`bool`): Determines Whether the handler should also accept
+            edited messages.
+        pass_args (:obj:`bool`): Determines whether the handler should be passed


Is there really a need to have the pass_ stuff on this new handler?

hmm, In my mind It would be best to keep all handlers compatible to the same model for v11.0 You disagree?

Kinda, but mostly from a "we shouldn't be writing code that's already deprecated" standpoint, but since you've already written it. I think it's fine :D

jsmnbom · 2018-05-21T14:51:35Z

telegram/ext/commandhandler.py

        context.args = check_result
+
+
+class PrefixHandler(CommandHandler):


Should we add a docstring note about arguments being availible on the CallbackContext object? (applies to CommandHandler too I suppose)

jsmnbom · 2018-05-21T14:52:10Z

telegram/ext/commandhandler.py

+
+    Attributes:
+        prefix (:obj:`str` | List[:obj:`str`]): The prefix(es) that will precede :attr:`command`.
+        command (:obj:`str` | List[:obj:`str`]): The command or list of commands this handler


Missing a full stop (.)

? On the next line at the end of the sentence...

Oh derp, sorry :P

jsmnbom · 2018-05-21T14:52:29Z

telegram/ext/commandhandler.py

+
+    Note:
+        :attr:`pass_user_data` and :attr:`pass_chat_data` determine whether a ``dict`` you
+        can use to keep any data in will be sent to the :attr:`callback` function.. Related to


I'll fix this in every handler then :D

jsmnbom · 2018-05-21T14:54:07Z

telegram/ext/inlinequeryhandler.py

                 pass_user_data=False,
-                 pass_chat_data=False):
+                 pass_chat_data=False,
+                 use_context=False):


This isn't right ...

leftover from my base. Fixed

jsmnbom · 2018-05-21T20:11:27Z

telegram/ext/commandhandler.py


    Commands are Telegram messages that start with ``/``, optionally followed by an ``@`` and the
-    bot's name and/or some additional text.
+    bot's name and/or some additional text. It will add a ``list`` to the :class:`CallbackContext`


"It" doesn't really make sense here (it works fine in the prefixhandler)

This reverts commit 87afd98.

…"" This reverts commit 9e2357b

jsmnbom and others added 30 commits May 3, 2018 23:18

Change handlers so context is supported

3ce9363

Attempt to make parameter "guessing" work on py < 3.5

714d96d

Document use_context in all handlers

32a2453

Add Context to docs

5738f00

Minor fixes to context handling

0580ae3

Add tests for context stuff

bad8f35

Allow the signature check to work on py<3.5 with methods

7e2fb8d

Fix order of operations

d133dd4

Address most issues raised in CR

5bfaca0

Make CommandHandler no longer support filter lists

bfec5ea

Fix indent

da26227

(pycharm can be an arse sometimes)

Improve readability in conversationhandler

c450a4e

Make context have Match instead of groups & groupdict

35c07dd

Remove filter list support from messagehandler too

b325cc1

Small fix to StringCommandHandler

945a5ec

More small fixes to handlers

07747f3

Amend CHANGES

a54cfac

Fix tests and fix bugs raised by tests

30712c5

Don't allow users to ignore errors without messing with the warning f…

e87a705

…ilters themselves

Merge branch 'master' into context

c69378f

Ignore our own deprecation warnings when testing

397ab77

Skipping deprecationwarning test on py2

46f2a26

Forgot some changes

fe27912

Handler: Improved documentation and text of deprecation warnings

0f44eed

HandlerContext: Keep only dispatcher and use properties; improved doc

b54f7e5

Complete fixing the documentation.

f26bf62

- Fixes per Eldinnie's comments. - Fixes per warnings when running sphinx.

Some small doc fixes (interlinks and optionals)

8184902

Change add_error_handler to use HandlerContext too

5dc53a5

Forgot about conversationhandler

256a9d1

jsmnbom and others added 5 commits May 10, 2018 18:16

Forgot jobqueue

63f8a07

Add tests for callbackcontext & for context based callback job

1fb2e6e

Commandhandler reworked

cf85625

Make CommandHandler strict

860155e

Only register valid botcommands, else raise ValueError

Add PrefixHandler

5f238d2

Eldinnie requested review from jsmnbom and tsnoam May 21, 2018 13:24

Eldinnie added 3 commits May 21, 2018 15:28

Merge branch 'master' into cmdhndlr

4fd74d9

declare encoding on test_commandhandler

eea3a6b

Fix some tests dependend on CommandHandler

72adb72

Eldinnie changed the title ~~rework ComamndHandler~~ rework CommandHandler May 21, 2018

jsmnbom reviewed May 21, 2018

View reviewed changes

CR changes

7e1642c

jsmnbom reviewed May 21, 2018

View reviewed changes

small docfix.

ae10983

jsmnbom approved these changes May 21, 2018

View reviewed changes

Test all possibilities for PrefixHandler

8840c2f

jsmnbom added the 📋 pending-merge work status: pending-merge label May 22, 2018

jsmnbom mentioned this pull request May 22, 2018

Added prefix argument to CommandHandler #1021

Closed

Eldinnie merged commit 87afd98 into master May 22, 2018

Eldinnie deleted the cmdhndlr branch May 22, 2018 19:44

Eldinnie added a commit that referenced this pull request May 31, 2018

Revert "CommandHandler overhaul and PrefixHandler added (#1114)"

9e2357b

This reverts commit 87afd98.

jsmnbom added a commit that referenced this pull request Sep 21, 2018

Revert "Revert "CommandHandler overhaul and PrefixHandler added (#1114)…

e75615c

…"" This reverts commit 9e2357b

jsmnbom added this to the V12 milestone Feb 9, 2019

Bibo-Joshi mentioned this pull request Jan 31, 2020

Make Filters.command only accept MessageEntity commands #1744

Merged

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

		context.args = check_result


		class PrefixHandler(CommandHandler):

Conversation

Eldinnie commented May 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsmnbom May 21, 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!

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

jsmnbom May 21, 2018 •

edited

Loading