Navigation Menu

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

[Freesound] extended and fix "title" extraction closes #11602 #11608

Closed
wants to merge 2 commits into from

Conversation

shizeeg
Copy link
Contributor

@shizeeg shizeeg commented Jan 5, 2017

Please follow the guide below

  • You will be asked some questions, please read them carefully and answer honestly
  • Put an x into all the boxes [ ] relevant to your pull request (like that [x])
  • Use Preview tab to see how your pull request will actually look like

Before submitting a pull request make sure you have:

In order to be accepted and merged into youtube-dl each piece of code must be in public domain or released under Unlicense. Check one of the following options:

  • I am the original author of this code and I am willing to release it under Unlicense
  • I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

  • Bug fix
  • Improvement
  • New extractor
  • New feature

Description of your pull request and other information

Extractor now fills more data and works correctly.

description = self._html_search_regex(
r'<div id="sound_description">(.*?)</div>', webpage, 'description',
fatal=False, flags=re.DOTALL)

duration = float_or_none(get_element_by_class('duration', webpage))
if duration:
duration = duration / 1000
Copy link
Collaborator

Choose a reason for hiding this comment

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

scale passed to float_or_none instead.

filesize = float_or_none(self._search_regex(
r'Filesize</dt><dd>(\d+.\d+).*</dd>', sound_info, 'file size (approx)', fatal=False))
if filesize:
filesize = filesize * 1048576
Copy link
Collaborator

Choose a reason for hiding this comment

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

parse_filesize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

parse_filesize() shows a bit smaller size (MB * 1000 instead of MB * 1024), but I don't think it's a big of an issue because it shows filesize approximately anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

MB stands for mega 10^6, not 2^20.
Also this file size has nothing to do with resulting mp3 file. It's probably for original wav.

Copy link
Contributor Author

@shizeeg shizeeg Jan 7, 2017

Choose a reason for hiding this comment

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

ah... 50.5 MB equals 48.7-something MiB you're correct. So it's fine.


download_count = int_or_none(self._html_search_regex(
r'Downloaded.*>(\d+)<', webpage, 'downloaded', fatal=False))
audio_url = self._og_search_property('audio', webpage, 'song url')
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be extracted in the first place.

formats = [{
'url': audio_url,
'id': music_id,
'format_id': self._og_search_property('audio:type', webpage, 'audio format'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not be fatal.

return {
'id': music_id,
'title': title,
'url': self._og_search_property('audio', webpage, 'music url'),
'title': self._og_search_property('audio:title', webpage, 'song title'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be extracted in the first place.

@shizeeg
Copy link
Contributor Author

shizeeg commented Jan 7, 2017

done.

@dstftw dstftw closed this in 3a407e7 Jan 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants