Open eikaramba opened 1 month ago
Thanks for this! I've left a few minor comments, although I'm not sure I'd want to merge this in its current form.
Currently it's just doing a title.contains(keyword)
to check if it matches, but that's not really sufficient as the keyword could be part of some other word in the title. For example, adding the word "trump" to the filter will filter out the word "trumpet", "trumping", etc. In other words this falls foul of the Scunthorpe problem.
I guess we'd want to split titles by punctuation/whitespace and check for matches that way?
I guess we'd want to split titles by punctuation/whitespace and check for matches that way?
It could be a phrase, like long series/book/etc title
thank you for having a look at the PR. yes you are absolutely right, i changed it so that the keywords are matched against complete words (using \W+, which matches one or more non-word characters (punctuation, whitespace, etc.))
I also resolved the aforementioned reviews apart from the refactoring, as i am unsure how you would like this to be done. i guess it would be good to refactor the whole class then and also make all the other conditions separate methods/classes.
Very nice. I see you put the option in Settings -> Behaviour -> Posts. I would instead suggest creating a whole new settings item, Settings -> Filters. Here we could then simply add more filters later (by Post domain, keyword, and Comment keyword etc. pp). Makes it easier to find and access in my opinion.
no problem i modified the PR to introduce a new settings category
this fixes https://github.com/QuantumBadger/RedReader/issues/646 and allows to specify a comma separated list of keywords which will be used to filter posts by title.
In the future of course regex or filter based on user flairs can be integrated.