Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rework CommandHandler #1114

Merged
merged 41 commits into from May 22, 2018

Conversation

@Eldinnie
Copy link
Member

Eldinnie commented May 21, 2018

  • Only register valid botcommands, else raise ValueError

  • Make CommandHandler strict
    Only register valid botcommands, else raise ValueError

  • Add PrefixHandler

jsmnbom and others added 30 commits May 3, 2018
(pycharm can be an arse sometimes)
 - 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
jsmnbom and others added 5 commits May 10, 2018
Only register valid botcommands, else raise ValueError
@Eldinnie Eldinnie requested review from jsmnbom and tsnoam May 21, 2018
@Eldinnie Eldinnie changed the title rework ComamndHandler rework CommandHandler May 21, 2018
@@ -119,6 +125,10 @@ def __init__(self,
self.command = [command.lower()]
else:
self.command = [x.lower() for x in command]
for comm in self.command:
if not re.match(r'^[\da-z_]{1,32}$', comm):

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 21, 2018

Member

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.

This comment has been minimized.

Copy link
@Eldinnie

Eldinnie May 21, 2018

Author Member

image
image
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

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 \

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 21, 2018

Member

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

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]

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 21, 2018

Member

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

This comment has been minimized.

Copy link
@Eldinnie

Eldinnie May 21, 2018

Author Member

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.

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:]

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 21, 2018

Member

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

This comment has been minimized.

Copy link
@Eldinnie

Eldinnie May 21, 2018

Author Member

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.

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 21, 2018

Member

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

"""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

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 21, 2018

Member

CommandHandler*

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

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 21, 2018

Member

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

This comment has been minimized.

Copy link
@Eldinnie

Eldinnie May 21, 2018

Author Member

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

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 21, 2018

Member

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

@@ -159,3 +169,147 @@ def collect_optional_args(self, dispatcher, update=None, check_result=None):

def collect_additional_context(self, context, update, dispatcher, check_result):
context.args = check_result


class PrefixHandler(CommandHandler):

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 21, 2018

Member

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

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

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 21, 2018

Member

Missing a full stop (.)

This comment has been minimized.

Copy link
@Eldinnie

Eldinnie May 21, 2018

Author Member

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

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 21, 2018

Member

Oh derp, sorry :P

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

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 21, 2018

Member

.. -> .

This comment has been minimized.

Copy link
@Eldinnie

Eldinnie May 21, 2018

Author Member

I'll fix this in every handler then :D

@@ -103,7 +103,8 @@ def __init__(self,
pass_groups=False,
pass_groupdict=False,
pass_user_data=False,
pass_chat_data=False):
pass_chat_data=False,
use_context=False):

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 21, 2018

Member

This isn't right ...

This comment has been minimized.

Copy link
@Eldinnie

Eldinnie May 21, 2018

Author Member

leftover from my base. Fixed

@@ -29,7 +29,9 @@ class CommandHandler(Handler):
"""Handler class to handle Telegram commands.
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`

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 21, 2018

Member

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

@Eldinnie Eldinnie merged commit 87afd98 into master May 22, 2018
5 checks passed
5 checks passed
Hound No violations found. Woof!
codecov/patch 100% of diff hit (target 92.69%)
Details
codecov/project 92.73% (+0.03%) compared to 247577b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Eldinnie Eldinnie deleted the cmdhndlr branch May 22, 2018
Eldinnie added a commit that referenced this pull request May 31, 2018
jsmnbom added a commit that referenced this pull request Sep 21, 2018
@jsmnbom jsmnbom added this to the V12 milestone Feb 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.