Skip to content

Making merged filters short-circuit #1350

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.

Sign up for GitHub

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

Jump to bottom
Merged
Eldinnie merged 2 commits into from
Mar 14, 2019
Merged

Making merged filters short-circuit #1350

Eldinnie merged 2 commits into from
Mar 14, 2019

Conversation

@jsmnbom
Copy link
Member

@jsmnbom jsmnbom commented Feb 18, 2019

This brings the behaviour back pre-datafilters.

Reasons:

  1. we use the bitwise | and & like the python and and or, and those keywords do short circuit.
  2. Good in cases where a custom filter has potential side effects or is slow
  3. Good to reduce redundancy if one has filters like Filters.reply & some_filter_that_assumes_reply_to_message_exists without needing to check explicitly if reply_to_message exsists in some_filter_that_assumes_reply_to_message_exists. Note that an explicit check (or try except ofc) cause otherwise the check will crash the thread if it is not a reply.

@jsmnbom jsmnbom requested a review from Eldinnie February 18, 2019 20:41
jsmnbom added a commit to jsmnbom/githubbotrevised that referenced this pull request Feb 19, 2019
@jsmnbom
Work around python-telegram-bot/python-telegram-bot#1350
b1b1e69
@Eldinnie
Copy link
Member

Eldinnie commented Feb 28, 2019

Code looks good to me.

However we do need to decide what we want to happen.
if:
Filter.regex('one') | Filters.regex('two')
and
message.text = 'one and two'
Do we want len(context.matches)==1 or len(context.matches)==2
The first is more pythonic in a sense that it is in line with the way python handles and and or and this PR changes behavior into that. However it might need some rewording on the context.matches docstring maybe?

@jsmnbom
Copy link
Member Author

jsmnbom commented Feb 28, 2019

I'd say that len(context.matches)==1 is the better option as I'm pretty sure we make a direct comparison between our bitwise operators and and or not in our docs.

Good idea about adding it to documentation. Maybe even adding a note directly in Filters.regex would be a good idea?

Eldinnie
Eldinnie approved these changes Mar 14, 2019
View reviewed changes
Copy link
Member

@Eldinnie Eldinnie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Eldinnie Eldinnie merged commit b5891a6 into master Mar 14, 2019
@Eldinnie Eldinnie deleted the mergedfilter-short-circuit branch March 14, 2019 08:03
@github-actions github-actions bot locked and limited conversation to collaborators Aug 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Eldinnie Eldinnie Eldinnie approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jsmnbom @Eldinnie