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

Don't make duplicate PatternsMatcher instances. #11

Merged
merged 4 commits into from
Apr 2, 2024
Merged

Conversation

wRAR
Copy link
Member

@wRAR wRAR commented Apr 1, 2024

Fixes #10

@codecov-commenter
Copy link

codecov-commenter commented Apr 1, 2024

Codecov Report

Merging #11 (f8f8667) into main (d7970ac) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head f8f8667 differs from pull request most recent head ea76c92. Consider uploading reports for the commit ea76c92 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #11      +/-   ##
==========================================
- Coverage   99.00%   99.00%   -0.01%     
==========================================
  Files           6        6              
  Lines         303      300       -3     
==========================================
- Hits          300      297       -3     
  Misses          3        3              
Files Coverage Δ
url_matcher/matcher.py 99.06% <100.00%> (-0.03%) ⬇️

@wRAR wRAR requested a review from BurnzZ April 1, 2024 14:34
@@ -35,7 +35,8 @@ def __init__(self, include: List[str], exclude: Optional[List[str]] = None, prio

def get_domains(self) -> List[str]:
domains = [get_pattern_domain(pattern) for pattern in self.include]
return [domain for domain in domains if domain]
# remove duplicate domains preserving the order
return list(dict.fromkeys(domain for domain in domains if domain))
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if preserving the order is important but why not.

@BurnzZ
Copy link
Member

BurnzZ commented Apr 1, 2024

Would this remove the need for checking the uniqueness of the identifier?

if matcher.identifier not in unique:
    ...

Ref: https://github.com/zytedata/url-matcher/pull/9/files#diff-badf0ec0ca4b614596811ab120a6d1dbae5348764dfbfec56c88b4956ecee20fR172

@wRAR
Copy link
Member Author

wRAR commented Apr 2, 2024

Yes.

@wRAR wRAR merged commit f15ea91 into main Apr 2, 2024
10 checks passed
@wRAR wRAR deleted the duplicate-matchers branch April 2, 2024 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid extra PatternsMatcher when using .add_or_update() with multiple include URL patterns
4 participants