Add support for regex video title filtering #425
Conversation
Update views.py Update tests.py Update source.html Update tasks.py Update signals.py Update 0001_initial.py Update models.py Update models.py Update tests.py
Fix pagenums for "only_skipped" query param
|
Also fixes a bug with the media.html pagination and adds support for triggering workflows from dispatch triggers. |
|
Thanks for the contribution! Mostly looks good, there's a couple of things that would probably need to be tweaked before it gets merged. I'll comment in-line on the commits. |
tubesync/sync/tasks.py
Outdated
| @@ -254,6 +255,11 @@ def download_media_metadata(media_id): | |||
| log.warn(f'Media: {source} / {media} is older than cap age ' | |||
| f'{max_cap_age}, skipping') | |||
| media.skip = True | |||
| # If the source has a search filter, check the video title matches the filter | |||
| if not re.search(source.filter_text,media.title): | |||
There was a problem hiding this comment.
Should this be an opt-in filter (only download media that match this regex) or an opt-out filter (skip media that match this regex)?
There was a problem hiding this comment.
I thought opt-in was more intuitive for a user. Currently the filter_text is specified, and then if anything does not match the filter, it is marked as a "skip". Happy to take your suggestion to this though as once I re-write this to use an is_regex_match(media_item_title) method it'll be easy to change later. Certainly regex supports inverse matching though it's clunky. The other way would be to specify both an "include" and an "exclude" string. I.e. include "foo" but exclude "bar" but it feels unnecessarily complex. On my local fork I'm already using a regex to download all videos which contain "40k" but don't contain "Darktide".
foo -> match
bar -> not matched
foo bar -> not matched
There was a problem hiding this comment.
I would imagine the most common use-case for this is going to be "I want to ignore specific videos on a channel". While you can do this with positive matching regexes having to use lots of (?!string$)'s everywhere isn't going to be that friendly.
Personally, I think this might be better as a exclude_regex field. What do you think?
There was a problem hiding this comment.
I'm not sure I follow. Negative lookaheads get this behaviour you describe. Inverting it is no work though I want to check that re supports negative lookaheads as I might need to swap it for re2. My primary use case is to do both (include the word "foo" but not include the word "bar") as above.
Regex string: ^(?!.*foo).*$
foo
bar <<< Match
foo bar
Regex string: foo
foo <<< Match
bar
foo bar <<< Match
There was a problem hiding this comment.
Why not add an option to specify what the filter does? Include/Exclude.
For various use cases having an allow-list vs an ignore-list might be very powerful.
Imagine a channel with thousands of videos, and perhaps a reoccuring title that appears throughout the channel, like "office update 15". It would be far easier to just set an include list of ^office update \d+$, imo.
Or perhaps a channel that is otherwise about some topic, but has music videos strewn about it.. You might set an exclude list of \(music\svideo\).
Writing regex is hard for some users and negations are tricky to reason about. Avoiding the need for them would be advantageous imo. Requiring one extra switch isn't a lot of work.
There was a problem hiding this comment.
I like this idea. Essentially just a dropdown or slider toggle "include/exclude" though so it's clear what behaviour is currently selected (tickboxes would be a bit ambiguous. I'll have a look later today or tomorrow at do a new branch to implement this.
Fixed whitespace Update tests.py Ran makemigrations Update models.py Update tests.py Update models.py Update tests.py Update models.py Update models.py Update tests.py Update models.py Update tests.py Update tests.py Update tests.py Update models.py Update models.py Update tests.py Update models.py Update models.py Update tests.py Update tests.py Update signals.py Update tasks.py Update signals.py Update models.py Update tasks.py Update signals.py Update tasks.py Update models.py
Updated according to comments on PR
|
Merged branch addressing all comments into this PR |
|
Thanks for your work here and sorry for the delay, I was away for a while. I'll review this and merge it into the next release. |
Thanks and no worries, I've also been away! I was working on a new branch to implement the toggle between include/exclude as suggested above, but it had a few bugs I couldn't track down so it hasn't been merged yet. Once I've tracked that down I'll submit a new PR to get feedback. |
|
Thanks! Submit a PR for that when you're ready, it'll probably be easier to review all in one. |
|
This PR introduced logic bug in the handling of skipping media (which is at best a mess right now so understandable) as well as a race condition where metadata was checked for the upload date before the metadata had been downloaded, marking every media item as initially skipped. I've pushed a fix here: You don't need to do anything and this seems to be working again, just mentioning for reference. |
|
How is this meant to work, in terms of filtering out videos? I suppose my regex is still going to match everything in this channel I'm processing with this: |
|
You can use negative lookaheads. I totally agree a switch toggle is more convenient, it was beyond my limited experience to complete quickly! As an example,
|
|
@locke4 Thanks for the detailed response! I'll give this ago when I start on another channel. I'm comfortable with regex in general but negative lookaheads are.. painful imo :D |
Feature addition to add filtering of video titles by regex, implementing #424
First time working with Django so please let me know any improvements that can be made! I looked into field validation and null handling but struggled to find a cleaner way to validate form input data. I also couldn't find a way to get the filter working for the test youtube channels in tests.py, so currently all tests run with the regex string "*.". I've tested it manually with a couple of channels and the regex string seems to work correctly.
I also needed to rework the "if not downloaded yet" behaviour in signals.py to ensure the behaviour of editing a source prior to an actual download occurring also supports this regex. I've done some testing and it seems like the behaviour is as expected but this may need more testing to ensure I haven't introduced any bugs by accident.