Removed innecesary function to convert subtitles, improved use of the youtube api #699

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

iemejia commented Feb 21, 2013

Removed innecesary function to convert subtitles to the srt format since the youtube api makes this conversion via the fmt=srt parameter.

Contributor

iemejia commented Feb 21, 2013

I had to change the hashes in the unit tests since the hashes compared against the xml subtitles and not the srt ones.

iemejia added some commits Feb 21, 2013

Added new option '--only-srt' to download only the subtitles of a video
Improved option '--srt-lang'
 - it shows the argument in case of missing subtitles
 - added language suffix for non-english languages (e.g. video.it.srt)
Refactor subtitle options from srt to the more generic 'sub'.
In order to be more consistent with different subtitle formats.
From:
* --write-srt to --write-sub
* --only-srt to --only-sub
* --all-srt to --all-subs
* --srt-lang to --sub-lang'

Refactored also all the mentions of srt for sub in all the source code.
Contributor

iemejia commented Feb 22, 2013

Actually I added some new options to deal with subtitles that I think can be quite useful. I hope you want to merge them. I took the liberty to change the 'srt' options for 'sub' because my commits support different subtitle formats.

@@ -18,3 +18,4 @@ youtube-dl.tar.gz
cover/
updates_key.pem
*.egg-info
+semantic.cache
@phihag

phihag Feb 24, 2013

Collaborator

What is this change for? Why are we writing that file?

Contributor

iemejia commented Feb 24, 2013

semantic.cache is a file that emacs generates for autocompletion, I just put it there, but now I think is not necessary since one can map, all the cache files to a different directory (a bit like with the ~ backup files), so please ignore just that commit. What do you think of the other changes ?

Collaborator

FiloSottile commented Feb 27, 2013

Oh, great! The xml ➡️ srt was the code I hated the most (and it was mine).

Let me have a look at the other changes. Thanks!

Contributor

iemejia commented Mar 19, 2013

Hey, it's been long time and I don't have any news if this is going to be accepted, or what can I do to get it accepted. I ask you since I think more people can find my contribution useful (that was the whole point of doing this pull request).

Collaborator

FiloSottile commented Mar 19, 2013

It LGTM! And it seems a bunch of useful features that will replace some ugly code.

@phihag Am I clear to rebase out the semantic.cache commit and merge?

Collaborator

phihag commented Mar 19, 2013

@FiloSottile Yeah, feel free to merge this one and other minor changes. I'm still dealing with the aftermath of my conference atm, but expect to be able to review everything else in the next days.

@@ -173,12 +173,24 @@ def _find_term_columns():
action='store', dest='format_limit', metavar='FORMAT', help='highest quality format to download')
video_format.add_option('-F', '--list-formats',
action='store_true', dest='listformats', help='list all available formats (currently youtube only)')
- video_format.add_option('--write-srt',
+ video_format.add_option('--write-sub',
@FiloSottile

FiloSottile Mar 20, 2013

Collaborator

Umh... this is a breaking change. @phihag should we add a hidden backwards compatibility --write-srt, or just specify the change in release notes?

@phihag

phihag Mar 20, 2013

Collaborator

Umh... this is a breaking change. @phihag should we add a hidden backwards compatibility --write-srt, or just specify the change in release notes?
Please stay backwards-compatible.

Collaborator

FiloSottile commented Mar 20, 2013

Here is the ready merged/rebased branch. It is a fast-forward from master.
https://github.com/rg3/youtube-dl/compare/subtitles

FiloSottile added a commit that referenced this pull request Mar 20, 2013

Merge pull request #699 by @iemejia
Removed innecesary function to convert subtitles, improved use of the youtube api
Collaborator

FiloSottile commented Mar 20, 2013

Merged! Thanks a lot @iemejia

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment