BrettSheleski / comchap

Commercial detection script to add chapters into video file
MIT License
136 stars 26 forks source link

Refactor Comcut and Comchat so common functionality is not repeated #6

Open BrettSheleski opened 7 years ago

BrettSheleski commented 7 years ago

Problem:

Currently Comcut and Comchap are largely the same and only really differ in operation after Comskip and generating the ffmeta file is done.

This leads to duplicate code. Duplicate code is bad code. This leads to functionality being added to one script and not to the other, or at least not consistently.

Solution:

Refactor Comcut and Comchap so that the common tasks such as setting up and running Comskip, generating the ffmeta file, file cleanup, etc. is implemented once and only once.

Implementation

Currently unsure what the best approach is to handle this.

robertclemens commented 7 years ago

Potentially you could just merge into a single script and specify --cut (comcut) to enter the cut phase and have the chapter markers only (comchap) as the default if not specified?

I suppose that is what I would do but there could be better options.

I also don't recall if you can tag the postprocessing field within Plex with any --command options or if you need to call a script to pass those to comcut/comchap.

BrettSheleski commented 7 years ago

I don't like the idea of merging the two to a single script for various reasons.

I also dont think you can specify command line arguments for Plex post processing. However being directly specified as a Plex post processing script is not a priority at all.

I'm thinking of moving all commonality (read command line arguments, etc) to a third file which both comcut and comchap will reference in some way and leaving comchap/comcut to do the nitty-gritty.

Reed97123 commented 7 years ago

I would suggest one underlying script that does everything, but with two separate front-end wrappers.

The back-end combined script would do everything depending on what options are put in from either the comchap or comcut wrappers. The comchap wrapper would retain it's same interface and would simply pass the appropriate comchap settings to the backend. Same for the comcut wrapper. The wrappers would be very simple just setting up and passing the appropriate arguments.

BrettSheleski commented 7 years ago

I made a "refactor" branch for this. I may push some things to it and see how I like it. If I like it enough I'll push the changes to master.

My thought is to have a libcomchap script that does all the common tasks defined in functions like read command line arguments, setup various variables, etc. Then within comcut and comchap I'd call the functions, then do their specific tasks, then call the cleanup functions.

I'm assuming I'd use the source command to do this. I'm just unsure how that would work with variable scopes across the two files. Experimentation will answer this. I believe it'll work as intended.

Also, my intent is to have all 3 files live in a lib folder (/usr/local/lib I believe) then add symlinks to the bin directory for just the comcut and comchap scripts. The scripts would have to figure out where they "really" live to source the libcomchap file in the lib directory.

The existing Makefile should work to install everything without needing alteration.

Reed97123 commented 7 years ago

I'm a little hesitant on needing an install for the script, but otherwise I think it's a good idea to combine some of the functionality and reduce code duplication.

BrettSheleski commented 7 years ago

The install process is really unnecessary and optional. If you like needing to specify the path to comcut/comchap, it's really unneeded. I just don't like neding to remember full paths to things and prefer to add symlinks to the /usr/local/bin directory.

I just made a Makefile to automate this for consistency. Just my preference. However it does introduce this little issue.