-
Notifications
You must be signed in to change notification settings - Fork 48
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
Support exclude filters #83
Conversation
@nning, just a friendly reminder to take a look at this PR. |
Thank you very much for your contribution! I just made some minor code styling comments and will test your feature after you made those changes. Would you also add tests, please (e.g. in |
I can't find the code styling comments you are referring to. I'm quite new to github, so it may just be that I don't know where to look for them. Sure, I will add tests, it is a reasonable requirement. |
@@ -103,7 +103,7 @@ def process_link(feed, item) | |||
# The link is not in +@seen+ Array. | |||
unless @seen.include?(link) | |||
# Skip if filter defined and not matching. | |||
unless feed.matches_regexp?(item.title) | |||
unless feed.matches_regexp?(item.title) && !(feed.exclude?(item.title)) |
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.
unless feed.matches_regexp?(item.title) && !(feed.exclude?(item.title)) | |
unless feed.matches_regexp?(item.title) && !feed.exclude?(item.title) |
return unless regexps.is_a?(Array) | ||
|
||
regexps.each do |regexp| | ||
matcher = regexp['matcher'] | ||
path = regexp['download_path'] | ||
exclude = regexp['exclude'] |
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.
exclude = regexp['exclude'] | |
exclude = regexp['exclude'] |
I'm sorry, I just realized, I did not submit my review. Would you please add tests? Thanks again for your contribution! |
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.
Please also add a configuration example in the README.
@@ -41,21 +42,30 @@ def matches_regexp?(title) | |||
@regexp.nil? || !(title =~ @regexp).nil? | |||
end | |||
|
|||
def exclude?(title) | |||
@excludes.each do |regexp, exclude| | |||
return title =~ to_regexp(exclude) if title =~ to_regexp(regexp) |
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.
The value of title =~ to_regexp(exclude)
should only be evaluated once.
When can we expect this pull request to be merged? |
This implements a new configuration option "exclude" for a regexp group. #29