jsmnbom · GitHub

@jsmnbom

Copy link Copy Markdown

Member

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

jsmnbom added a commit to jsmnbom/githubbotrevised that referenced this pull request

Feb 19, 2019

@Eldinnie

Copy link Copy Markdown

Member

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 Copy Markdown

Member Author

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?

@jsmnbom

Eldinnie

Copy link Copy Markdown

Member

Choose a reason for hiding this comment

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

lgtm

@Eldinnie

@Eldinnie

Eldinnie deleted the mergedfilter-short-circuit branch

March 14, 2019 08:03

Read the original on github.com ↗