-
Notifications
You must be signed in to change notification settings - Fork 10
refactor: Remove extra should_include check and combine regex patterns #575
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,48 +6,63 @@ class Matcher: | |||||||||||||||||||||||||
| def __init__(self, patterns: Sequence[str] | None): | ||||||||||||||||||||||||||
| self._patterns = set(patterns or []) | ||||||||||||||||||||||||||
| self._is_initialized = False | ||||||||||||||||||||||||||
| # a list of patterns that will result in `True` on a match | ||||||||||||||||||||||||||
| self._positives: list[re.Pattern] = [] | ||||||||||||||||||||||||||
| # a list of patterns that will result in `False` on a match | ||||||||||||||||||||||||||
| self._negatives: list[re.Pattern] = [] | ||||||||||||||||||||||||||
| self._combined_positives: re.Pattern | None = None | ||||||||||||||||||||||||||
| self._combined_negatives: re.Pattern | None = None | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| def _get_matchers(self) -> tuple[list[re.Pattern], list[re.Pattern]]: | ||||||||||||||||||||||||||
| def _get_matchers(self) -> tuple[re.Pattern | None, re.Pattern | None]: | ||||||||||||||||||||||||||
| if not self._is_initialized: | ||||||||||||||||||||||||||
| positive_patterns = [] | ||||||||||||||||||||||||||
| negative_patterns = [] | ||||||||||||||||||||||||||
| for pattern in self._patterns: | ||||||||||||||||||||||||||
| if not pattern: | ||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||
| if pattern.startswith(("^!", "!")): | ||||||||||||||||||||||||||
| self._negatives.append(re.compile(pattern.replace("!", ""))) | ||||||||||||||||||||||||||
| negative_pattern = pattern.replace("!", "") | ||||||||||||||||||||||||||
| negative_patterns.append(negative_pattern) | ||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||
| self._positives.append(re.compile(pattern)) | ||||||||||||||||||||||||||
| positive_patterns.append(pattern) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| # Combine positive patterns into a single regex for faster matching | ||||||||||||||||||||||||||
| if len(positive_patterns) > 0: | ||||||||||||||||||||||||||
| # Combine patterns with OR, each pattern is wrapped in parentheses to preserve anchors | ||||||||||||||||||||||||||
| combined_pattern = ( | ||||||||||||||||||||||||||
| "|".join(f"({p})" for p in positive_patterns) | ||||||||||||||||||||||||||
| if len(positive_patterns) > 1 | ||||||||||||||||||||||||||
| else positive_patterns[0] | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| self._combined_positives = re.compile(combined_pattern) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
| if len(positive_patterns) > 0: | |
| # Combine patterns with OR, each pattern is wrapped in parentheses to preserve anchors | |
| combined_pattern = ( | |
| "|".join(f"({p})" for p in positive_patterns) | |
| if len(positive_patterns) > 1 | |
| else positive_patterns[0] | |
| ) | |
| self._combined_positives = re.compile(combined_pattern) | |
| if positive_patterns: | |
| self._combined_positives = re.compile( | |
| "|".join(positive_patterns) | |
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we not need to preserve possible anchors here in the regex pattern with parens around each, @thomasrockhu-codecov?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@calvin-codecov sorry, not sure what you mean here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh nvm, there's actually no problem here
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of scope for this PR, but can we add a ticket to just call self._get_matchers() at __init__ time? This means we don't need to call it here and can just pull self._combined_positives etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if combined_negatives: | |
| if combined_negatives.match(s): | |
| return False | |
| if combined_negatives is not None and combined_negatives.match(s): | |
| return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you COULD do
return bool(combined_positives.match(s)) if combined_positives else True
but, that's a style thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with keeping this in this form for clarity at quick glance as each of these 3 scenarios are explicit since we also have the combined_negatives clause right above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.