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

Context #1100

Merged
merged 33 commits into from May 21, 2018

Conversation

@jsmnbom
Copy link
Member

jsmnbom commented May 3, 2018

This adds optional support for using context objects instead of all the pass_* in the handlers, as we've talked about a lot in the past.
Motivation is #1080 being seemingly really difficult to implement nicely, and it seems redundant if this got implemented instead (therefore closes #1080).
Not sure if this is exactly how we talked about it, but it seems to implement all that's needed (and as a bonus is 100% backwards compatible!!).

Currently employs a bit of magic to figure out whether to use the old pass_ or the new context. Namely it checks the callback signature to check how many parameters it expects. This is expected to work very well on python >= 3.5 even with decorators. On python < 3.5 it will fail completely if decorators are used, but otherwise work fine.
The user can disable the new functionality by setting use_context=False in a given handler. This is also how it can be manually turned on, in the case of python < 3.5 and decorated callback functions.

@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented May 3, 2018

The design discussion is at #1026

@jsmnbom

This comment has been minimized.

Copy link
Member Author

jsmnbom commented May 3, 2018

This is a bit different from that discussion. Mainly that everything is in the context object, but that it acts like a update object, so it's still very convenient for the user. (it also made it much easier to do backwards compat, since it's a different amount of parameters)
The only thing that stood out to be as something we maybe should change is whether to keep groups/groupdict or just have the Match object in there instead.

jsmnbom added 8 commits May 3, 2018
@tsnoam tsnoam force-pushed the context branch from 9b0bda4 to d133dd4 May 3, 2018
Copy link
Member

tsnoam left a comment

This looks much better, but I'm afraid we'll still need another pass, hopefully the last. See my comments on the code.

Regarding the "user facing strings" (docstring, deprecation warnings & changelog) - once we'll get a version of the code we're all happy with, I'll add a commit with text (and then you can scrutinize me, lol ;-) )

handler.handle_update(update, self)
for handler in self.handlers[group]:
check = handler.check_update(update)
if check is not None and check is not False:

This comment has been minimized.

Copy link
@tsnoam

tsnoam May 4, 2018

Member

As discussed, I find this semantics confusing. I prefer something more straight forward:
bool(checkl) evaluates to False - don't handle the update.
bool(check) evaluates to True - handle the update and pass check to handle_update().

However, if you can describe the semantics you chose in simple English and a short phrase (and preferably with docstrings type annotations which makes sense) we can consider your way.

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 5, 2018

Author Member

Current behaviour I've described as:
Either ``None`` or ``False`` if the update should not be handled. Otherwise an object that will be passed to :attr:`handle_update` and :attr:`collect_additional_context` when the update gets handled.
This allows both stuff like having CommandHandle.check_update() return message.text.split()[1:] when the update should be handled, and even if there are no args, and it therefore returns [] (a falsey value) it will be handled.
This would advokate for having it just be: Either None to not handle or something else to handle.
But for example in MessageHandler.check_update() I have a return self.filters(message) which determines if the update should be handled.

In conclusion I think we should keep the current logic. Or if you really wanna simplify it, then have it be None to not handle, and everything else gets handled (so the first scenario is still supported).

@@ -63,12 +78,25 @@ def __init__(self,
pass_update_queue=False,
pass_job_queue=False,
pass_user_data=False,
pass_chat_data=False):
pass_chat_data=False,
use_context=None):

This comment has been minimized.

Copy link
@tsnoam

tsnoam May 4, 2018

Member

We should put a (very short) deadline for use_context=None and move to an extra deprecation period with use_context=True.

This comment has been minimized.

Copy link
@tsnoam

tsnoam May 4, 2018

Member

So how long do you propose to have this default to old handler semantics (with deprecation warning obviously) and how long do you think we should keep backward compatibility code once we change the default to the new handler semantics?

This comment has been minimized.

Copy link
@Eldinnie

Eldinnie May 4, 2018

Member

I would say 2 months for both?

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 5, 2018

Author Member

1 month for each of them seems fine to me... or is that what you're suggesting too?

This comment has been minimized.

Copy link
@tsnoam

tsnoam May 5, 2018

Member

Maybe: Next major version (so if this is going to be v11, then on v12 we change the default to True, and on v13 or v14 we'll remove the backward compatibility code)

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 5, 2018

Author Member

Sounds good to me

@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented May 5, 2018

Replying to @Eldinnie review:
1,1. I agree for CommandHandler to be only for MessagEntity.COMMAND.
Can be in the next PR, but must be before release.

1.2. MessageHandler for everything else. PrefixHandler should be a convenience wrapper on top of it.
Can be in the next PR, but must be before release.

1.3. What is wrong with list of filters? I do not recall that conversation.

2.1. *_updates should still exist in MessageHandler.init() prototype. The implementation detail of using filters is a very good idea.
Can be in the next PR, doesn't have to be before release.

2.2. I would keep RegexHandler, but change it to be a convinient wrapper on top of MessageHandler.

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented May 5, 2018

1.3 Long time ago the filters argument was supposed to be a list of callables and the handler woud handle the update if any of those would return True In october 2016 the bitwise filters have been added #411 and we've supported both (list of filters and bitwise filters) since then. there's a warning list is deprecated since then: https://github.com/python-telegram-bot/python-telegram-bot/blob/master/telegram/ext/messagehandler.py#L123
I say we remove it now

@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented May 5, 2018

1.3. Ok. Removing the filters list is a good idea. But I think that we should have in the migration guide a code snippet demonstrating how to convert a list of filters to bitwise-or filters

@JosXa

This comment has been minimized.

Copy link
Contributor

JosXa commented May 5, 2018

Could we please gather up the individual user handlers that do not fit the new callback signature and show a single deprecation warning message to the user in the following fashion:

"TelegramDeprecationWarning: You are currently not using context based handlers. This is being deprecated, and will soon not be possible. Please change your callback function signatures from (bot, update, others...) to (update, context), and create your handlers using use_context=True instead of setting any pass_ parameters. You can set use_context=False to turn this warning off. Please see https://git.io/vpVe8 for more info.

Your following callables are affected by this:
- MessageHandler(whatever_the_user_set_up, pass_bla=True)
- MessageHandler(whatever_the_user_set_up, pass_bla=True)
- MessageHandler(whatever_the_user_set_up, pass_bla=True)
- MessageHandler(whatever_the_user_set_up, pass_bla=True)
- MessageHandler(whatever_the_user_set_up, pass_bla=True)
- MessageHandler(whatever_the_user_set_up, pass_bla=True)
- MessageHandler(whatever_the_user_set_up, pass_bla=True)
- MessageHandler(whatever_the_user_set_up, pass_bla=True)
- ...
"

Might be possible to do this with a little timeout that checks a list of wrongdoings after like 2 seconds or so after the initialization step. Reasoning behind this is that I'm currently being flooded with ~140 warning messages in one of my bigger bots, and we don't want to make a bad impression towards our users with this change but the opposite :)

@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented May 5, 2018

@JosXa Lets first see what the actual strings will look like.
As stated on the developers group, I want them to look something like:

<object_name>  is deprecated. See http://...
jsmnbom added 2 commits May 5, 2018
@python-telegram-bot python-telegram-bot deleted a comment from tsnoam May 11, 2018
@python-telegram-bot python-telegram-bot deleted a comment from tsnoam May 11, 2018
@python-telegram-bot python-telegram-bot deleted a comment from tsnoam May 11, 2018
@python-telegram-bot python-telegram-bot deleted a comment from tsnoam May 11, 2018
@python-telegram-bot python-telegram-bot deleted a comment from tsnoam May 11, 2018
@python-telegram-bot python-telegram-bot deleted a comment from Eldinnie May 11, 2018
@python-telegram-bot python-telegram-bot deleted a comment from tsnoam May 11, 2018
@python-telegram-bot python-telegram-bot deleted a comment from tsnoam May 11, 2018
@python-telegram-bot python-telegram-bot deleted a comment from tsnoam May 11, 2018
@python-telegram-bot python-telegram-bot deleted a comment from tsnoam May 11, 2018
query = update.shipping_query
# check the payload, is this from your bot?
if query.invoice_payload != 'Custom-Payload':
# answer False pre_checkout_query
bot.answer_shipping_query(shipping_query_id=query.id, ok=False,
error_message="Something went wrong...")
query.answer(shipping_query_id=query.id, ok=False,

This comment has been minimized.

Copy link
@nmlorg

nmlorg May 15, 2018

Contributor

Remove shipping_query_id=query.id, .

@@ -89,31 +90,33 @@ def shipping_callback(bot, update):
# an array of LabeledPrice objects
price_list = [LabeledPrice('B1', 150), LabeledPrice('B2', 200)]
options.append(ShippingOption('2', 'Shipping Option B', price_list))
bot.answer_shipping_query(shipping_query_id=query.id, ok=True,
shipping_options=options)
query.answer(shipping_query_id=query.id, ok=True,

This comment has been minimized.

Copy link
@nmlorg

nmlorg May 15, 2018

Contributor

Remove shipping_query_id=query.id, .

query = update.pre_checkout_query
# check the payload, is this from your bot?
if query.invoice_payload != 'Custom-Payload':
# answer False pre_checkout_query
bot.answer_pre_checkout_query(pre_checkout_query_id=query.id, ok=False,
error_message="Something went wrong...")
query.answer(pre_checkout_query_id=query.id, ok=False,

This comment has been minimized.

Copy link
@nmlorg

nmlorg May 15, 2018

Contributor

Remove pre_checkout_query_id=query.id, .

else:
bot.answer_pre_checkout_query(pre_checkout_query_id=query.id, ok=True)
query.answer(pre_checkout_query_id=query.id, ok=True)

This comment has been minimized.

Copy link
@nmlorg

nmlorg May 15, 2018

Contributor

Remove pre_checkout_query_id=query.id, .

optional_args = self.collect_optional_args(dispatcher, update)

return self.callback(dispatcher.bot, update, **optional_args)
if self.filters is None:

This comment has been minimized.

Copy link
@nmlorg

nmlorg May 15, 2018

Contributor

self.filters will never be None if it evaluated to True on line 149.

self.use_context = False

self._dispatcher = MockDispatcher()
self._dispatcher = None

This comment has been minimized.

Copy link
@nmlorg

nmlorg May 15, 2018

Contributor

This clobbers lines 50-59, maybe reorder or:

  if bot:
   ...
      self._dispatcher = MockDispatcher()
! else:
!     self._dispatcher = None
@jsmnbom

This comment has been minimized.

Copy link
Member Author

jsmnbom commented May 15, 2018

Thanks for the review @nmlorg :D You're totally right in all the comments :)

@jsmnbom jsmnbom merged commit 247577b into master May 21, 2018
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
Hound No violations found. Woof!
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Eldinnie Eldinnie deleted the context branch May 21, 2018
Eldinnie added a commit that referenced this pull request May 31, 2018
This reverts commit 247577b.
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
5 participants
You can’t perform that action at this time.