always expect exists filter #480
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #480 +/- ##
==========================================
+ Coverage 92.24% 92.34% +0.09%
==========================================
Files 130 130
Lines 9486 9492 +6
==========================================
+ Hits 8750 8765 +15
+ Misses 736 727 -9 ☔ View full report in Codecov by Sentry. |
| @@ -126,15 +130,36 @@ def _add_exists_filter(parsed_rules: list): | |||
| for parsed_rule in parsed_rules: | |||
| temp_parsed_rule = parsed_rule.copy() | |||
| skipped_counter = 0 | |||
|
|
|||
| for segment_index, segment in enumerate(temp_parsed_rule): | |||
There was a problem hiding this comment.
Could you maybe flatten the if-statements in this loop? That way it is a bit easier to understand. A suggestion cold look like this:
for segment_idx, segment in enumerate(temp_parsed_rule):
if isinstance(segment, (Exists, Always)):
skipped_counter += 1
continue
exists_filter = RuleParser._get_exists_filter(segment)
if exists_filter is None:
skipped_counter += 1
continue
if exists_filter in filter_expression:
skipped_counter += 1
continue
filter_expression.insert(segment_idx * 2 - skipped_counter, exists_filter)| skipped_counter += 1 | ||
| continue | ||
| else: | ||
| parsed_rule.insert(segment_idx * 2 - skipped_counter, exists_filter) |
There was a problem hiding this comment.
Can you explain where the 2 is coming from? I was trying to understand this and my first idea was that segment_idx - 1 should work as well, but it doesn't. Maybe you could give this magic number a meaningful name?
There was a problem hiding this comment.
It was a way to account for the new elements added to the list.
segment_idx * 2 - skipped_counter
<=> segment_idx + (segment_idx - skipped_counter)
<=> segment_idx + added_exists_filter_count
It was indeed confusing, so I changed it.
| if isinstance(segment, Not): | ||
| not_child = segment.children[0] | ||
| if isinstance(not_child, KeyValueBasedFilterExpression): | ||
| return Exists(not_child.key) | ||
| elif isinstance(segment, KeyValueBasedFilterExpression): | ||
| return Exists(segment.key) | ||
| return None |
There was a problem hiding this comment.
How about we use this implementation? It's a bit easier to read and understand by preventing nested if-statements.
| if isinstance(segment, Not): | |
| not_child = segment.children[0] | |
| if isinstance(not_child, KeyValueBasedFilterExpression): | |
| return Exists(not_child.key) | |
| elif isinstance(segment, KeyValueBasedFilterExpression): | |
| return Exists(segment.key) | |
| return None | |
| if isinstance(segment, Not): | |
| segment = segment.children[0] | |
| if isinstance(segment, KeyValueBasedFilterExpression): | |
| return Exists(segment.key) | |
| return None |
There was a problem hiding this comment.
Thanks for the suggestion. I've changed it.
See issue #435