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

Making merged filters short-circuit #1350

Merged
merged 2 commits into from Mar 14, 2019

Conversation

@jsmnbom
Copy link
Member

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 Feb 18, 2019
jsmnbom added a commit to jsmnbom/githubbotrevised that referenced this pull request Feb 19, 2019
@Eldinnie

This comment has been minimized.

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

This comment has been minimized.

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?

Copy link
Member

Eldinnie left a comment

lgtm

@Eldinnie Eldinnie merged commit b5891a6 into master Mar 14, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Eldinnie Eldinnie deleted the mergedfilter-short-circuit branch Mar 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.