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

[Weibo] add support for Sina Weibo #15079

Merged
merged 13 commits into from
Jan 9, 2018
Merged

[Weibo] add support for Sina Weibo #15079

merged 13 commits into from
Jan 9, 2018

Conversation

sprhawk
Copy link
Contributor

@sprhawk sprhawk commented Dec 26, 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

add support for downloading video from Sina Weibo (Weibo.com), both URL from desktop site or mobile site

Copy link
Collaborator

@yan12125 yan12125 left a comment

Choose a reason for hiding this comment

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

This is good in general, just some comments for minor issues

'Accept-Language': 'en,zh-CN;q=0.9,zh;q=0.8',
'User-Agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.84 Safari/537.36',
'Upgrade-Insecure-Requests': '1',
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are all of these necessary? I think youtube-dl defaults suffice.

'Upgrade-Insecure-Requests': '1',
}
# to get Referer url for genvisitor
webpage, urlh = self._download_webpage_handle(url, video_id, headers=headers, note="first visit the page")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use _ (underline) instead of webpage if the value is not used.


data = urlencode({
"cb": "gen_callback",
"fp": '{"os":"2","browser":"Gecko57,0,0,0","fonts":"undefined","screenInfo":"1440*900*24","plugins":""}',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use json.dumps instead

'Referer': visitor_url,
}

r_genvisitor = request.Request(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use data and headers parameters of _download_webpage instead.

data=data,
headers=headers,
)
webpage, urlh = self._download_webpage_handle(r_genvisitor, video_id, note="gen visitor")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use _download_webpage is urlh is not used. And note should be meaningful for typical users.

'Accept-Encoding': 'gzip, deflate, br',
'Accept-Language': 'en,zh-CN;q=0.9,zh;q=0.8',
'Upgrade-Insecure-Requests': '1',
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same issue

'Upgrade-Insecure-Requests': '1',
}
# to get Referer url for genvisitor
webpage, urlh = self._download_webpage_handle(url, video_id, headers=headers, note="visit the page")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same issue for urlh

'url': page_info['media_info']['stream_url'],
'format': 'mp4',
}
formats = [format]
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can just use 'url' in the final dict if there's only one format

'format': 'mp4',
}
formats = [format]
uploader = weibo_info['status']['user']['screen_name']
Copy link
Collaborator

Choose a reason for hiding this comment

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

'title': title,
'uploader': uploader,
'formats': formats
# TODO more properties (see youtube_dl/extractor/common.py)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same

@sprhawk
Copy link
Contributor Author

sprhawk commented Dec 29, 2017

@yan12125 when can you do a further review over the new changes?

@yan12125
Copy link
Collaborator

when can you do a further review over the new changes?

jsonp stuffs are still there, and in some cases _download_webpage_handle can be changed to _download_webpage

@sprhawk
Copy link
Contributor Author

sprhawk commented Dec 29, 2017

@yan12125 Sorry, missed your reply. Changed them

@sprhawk
Copy link
Contributor Author

sprhawk commented Jan 3, 2018

@yan12125 I think the travis-ci erros are not related to my code.

@yan12125
Copy link
Collaborator

yan12125 commented Jan 4, 2018

I've restarted the build.

@yan12125 yan12125 merged commit 6648fd8 into ytdl-org:master Jan 9, 2018
yan12125 pushed a commit that referenced this pull request Jan 9, 2018
@yan12125
Copy link
Collaborator

yan12125 commented Jan 9, 2018

Thanks for the extractor! Do you want to be listed in AUTHORS? If so under what name?

@sprhawk
Copy link
Contributor Author

sprhawk commented Jan 9, 2018 via email

@yan12125
Copy link
Collaborator

yan12125 commented Jan 9, 2018

Done :)

yan12125 pushed a commit that referenced this pull request Jan 9, 2018
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.

2 participants