caronc / nzb-subliminal

Fetches subtitles for the videos it's provided. It can be easily integrated into NZBGet and SABnzbd too.
GNU General Public License v3.0
103 stars 14 forks source link

No files found if file path contains a comma #59

Closed sguillope closed 6 years ago

sguillope commented 6 years ago

If a path contains a comma , it will get split into 2 by ScriptBase.parse_path_list. This causes a problem when trying to scan a directory which name contains a comma (like for this movie)

So imagine I run Subliminial.py "/path/to/my, directory" from /home, the script will end up treating /path/to/my, directory as 2 paths: /path/to/my and /home/directory.

caronc commented 6 years ago

That link you shared isn't working for me unfortunately. But with respect t your question: The parse_path_list() is only used when passing in arguments from the script, so it shouldn't impact any video directories.

So imagine I run Subliminial.py "/path/to/my, directory" from /home, the script will end up treating /path/to/my, directory as 2 paths: /path/to/my and /home/directory.

That's 100% correct :). I can't think of a reason why you might want to store videos in a root directory that has a comma in it. Hence:

# If your video library was structured like this:
/a/path/to/your/movie/library/A,Video,Directory,With,Commas/your.video.here.mkv
                         ^
                         |
  You would scan this path (of this script) in you root when using this script.

Alternatively you can just change to the comma,directory and scan relative to where you are:

cd /a/path/to/your/movie/library/A,Video,Directory,With,Commas/
./Subliminal.py -S *

If you really see this as an issue, perhaps i can add an option to allow you to 'escape' the comma? So... something like this:

## double backslash prefix to comma would prevent a directory split
## \\,
Subliminal.py -S /a/path/to/your/movie/library/A\\,Video\\,Directory\\,With\\,Commas/your.video.here.mkv

Thoughts?

sguillope commented 6 years ago

Hey Chris, thanks for the detailed reply, it all makes sense. However let me give you a bit of context on my setup.

I'm running Sonarr/Radarr which automatically does the moving and renaming of files once they are downloaded. Files are moved under a folder with the name of the series/movie and year. In this case the directory is I, Tonya (2017). To debug the other issues I was encountering, I ended running Subliminial.py manually on the aforementioned directory. Had it be run by NZBGet during post-processing (and thus before the move by Radarr) it would not have been a problem as there was no comma anywhere then.

I understand that having a comma in a file path is not very conventional but it's still valid. Now to be honest it's probably not very common so I don't feel strongly about it but figured I'd report the potential issue as it took me a while (with some debugging) to realise what the problem was. I realise now that it's clearly documented in the README but I hadn't read that part :(

Note that in my case I was passing the path at the end of the command (as opposed to using the -S switch) which doesn't really need to support the split on , or | since you can simply append multiple paths separated by a space.

Feel free to close :)

caronc commented 6 years ago

I understand that having a comma in a file path is not very conventional but it's still valid. ... Note that in my case I was passing the path at the end of the command (as opposed to using the -S switch) which doesn't really need to support the split on , or | since you can simply append multiple paths separated by a space.

This is an interesting suggestion! So only do the directory parsing on entries associated with the -S switch and not stand-alone arguments... hmm; that's a good idea.

Right now i think i lump everything together and throw it into the _ScriptBase.parse_pathlist for simplicity. I'll need to peak in the code and see how easy it would be to instead selectively call this function. It may or may not be trivial to do.

I'm running Sonarr/Radarr which automatically does the moving and renaming of files once they are downloaded. Files are moved under a folder with the name of the series/movie and year. In this case the directory is I, Tonya (2017).

Just as a suggestion, consider that Sonarr/Radarr will eventually move your videos to /some/library/that/probably/does/not/have/a/comma/into/it, Subliminal is smart enough to scan your entire library (recursively) while not abusing or searching for subs on items that simply don't have them (it gives up after 24 hours by default - which is configurable). What I'm getting at is if you (remove reference to it from Radarr/Sonarr and) just set a cron to run over your base library periodically, Subliminal will do all of the heavy lifting for you (regardless of comma's found in the paths).

# $> crontab -e
# Check every hour for subs
# By default this will skip over videos with embedded subs
# Only look for subs for up to 24 hours (based on the date of the video)
#     - if you want to change this, use -A 48 (to change it to 48 hours)
#     - touch the video to reset it's time window is another option to:

/1 * * * * /path/to/Subliminal.py -s /usr/share/TVShows /usr/share/Movies

I realize you don't get the handy feature of having it search once right after the download with the above solution, But for newer content (like newly aired TV Shows), this is essential simply because there may not be subs generated yet for the content you're retrieving. Anyway.. just a side note.

As a very final note, i'm not sure what the options look like in Radarr/Sonarr; but another work around could just be to change the reference to calling Subliminal:

# from
Subliminal.py /absolute/path/to/video.mkv

# to
cd $(basename /absolute/path/to/video.mkv) && Subliminal.py .

# or
cd /absolute/path/to/video && Subliminal.py .
caronc commented 6 years ago

Turns out it wasn't too difficult of a patch to not parse paths specified without use of the --scandir= (-S) switch. The fix only applies to CLI calls (so this shouldn't break the tool for SABnzbd users).

# The following will scan the directory (if existing):
#   - /path/to/directory/with,commas,here
Subliminal.py /path/to/directory/with,commas,here

# Legacy support still enforced as the following will scan the directories (if existing):
#   - /path/to/directory/with
#   - commas
#   - here
Subliminal.py -S /path/to/directory/with,commas,here

Edit : give the master branch another checkout and let me know how it goes.

caronc commented 6 years ago

@sguillope : I hadn't heard from you in a bit. I'm going to close this issue now that I've applied your recommendation. If you do have any issues, feel free to comment here and I'll re-open it (the ticket).