Skip to content
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

Fix static extension #19

Closed
wants to merge 4 commits into from
Closed

Conversation

thrdroom
Copy link

This changes should fix #18

I removed the hardcoded mkv file extension from the sync function and implemented a remux function in case youtube-dl is downloading a mp4 instead of a mkv. This method also opens the possibility of allowing more containers other than mkv in the future.

I also outsourced the default format into a separate settings file, for better usability. This way the user don't have to change the source code of the youtube-sync file to change the default format.

@PotcFdk PotcFdk self-assigned this Jul 31, 2019
@@ -85,7 +87,11 @@ function set_default_option {
}

function set_default_options {
set_default_option "$1" "format" "bestvideo+bestaudio/best"
defaultformat="$(cat "default-format")"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest the path META/default-format instead of ./default-format

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

besides that I'd prefer to stay consistent with how the script handles options. that would mean to create the option file, e.g. set_default_global_option "default-format" "bestvideo+bestaudio/best".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to create the option file

oh, I now realize you've committed the option file to the repo. I wouldn't do that. Besides, cat "default-format" refers to ./default-format, and the yt-sync script doesn't neccessarily need to resist in the PWD.

Copy link
Author

@thrdroom thrdroom Aug 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest the path META/default-format instead of ./default-format

If i understand correctly the META directory is for saving the metadata about the channels and videos...i dont understand exactly why saving the config-file in an extra META directory would have any advantage. But i also don't really care about how the maintainer wants to structure the codebase. So, just let me know how and why you want it ;)

@myrdd
Copy link
Contributor

myrdd commented Aug 6, 2019

fwiw, there are some spelling errors:
allready > already
successfull > successful

@thrdroom
Copy link
Author

@PotcFdk Can i get your jabber-id to chat with you directly? havent found a way to contact you on your website.

@PotcFdk
Copy link
Owner

PotcFdk commented Oct 22, 2019

@thrdroom I haven't had the time to do the required maintenance on my XMPP daemon yet, but I can offer IRC instead. PotcFdk on IRCNet

@PotcFdk
Copy link
Owner

PotcFdk commented Oct 22, 2019

The results of the discussion can be seen in #11.

@PotcFdk PotcFdk closed this Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Embedding subtitles ends in ERROR: Conversion failed!
3 participants