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

[bbc] Fix errors #23415

Closed
wants to merge 9 commits into from
Closed

[bbc] Fix errors #23415

wants to merge 9 commits into from

Conversation

ajj8
Copy link

@ajj8 ajj8 commented Dec 15, 2019

Fixes errors on news articles, archive pages, etc

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

Fixes the errors on news article pages, archive pages, etc.

Fixes errors on news articles, archive pages, etc
@ajj8
Copy link
Author

ajj8 commented Dec 15, 2019

Fixes #23232

@ajj8 ajj8 mentioned this pull request Mar 16, 2020
5 tasks
@ajj8
Copy link
Author

ajj8 commented Mar 17, 2020

@dstftw @remitamine
Please can you consider merging this? There are many issues fixed by this pull.

Comment on lines 986 to 988
r'"vpid":"(%s)"' % self._ID_REGEX,
r'"versionPid":"(%s)"' % self._ID_REGEX,
r'"pid":"(%s)"' % self._ID_REGEX],
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Provide concrete example URLs for each pattern. Add tests.
  2. Relax regexes.

Copy link
Author

@ajj8 ajj8 Mar 29, 2020

Choose a reason for hiding this comment

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

  1. "vpid" parameter is used for pre-existing test https://www.bbc.co.uk/bbcthree/clip/73d0bbd0-abc3-4cea-b3c0-cdae21905eb1
    "pid" parameter is used for pre-existing test http://www.bbc.com/sport/0/football/33653409
    Comments for these previously broken test have been amended to represent this. I can't remember what "versionPid" applied to so I removed it.
  2. Relax in what way? I think this adequately captures programme ID from standard JSON data?

youtube_dl/extractor/bbc.py Outdated Show resolved Hide resolved
youtube_dl/extractor/bbc.py Outdated Show resolved Hide resolved
@ajj8 ajj8 requested a review from dstftw April 8, 2020 13:00
@ajj8
Copy link
Author

ajj8 commented Jun 2, 2020

@dstftw @remitamine I have not seen any activity on this for weeks, please can you help move this along?

@ajj8
Copy link
Author

ajj8 commented Jul 14, 2020

Still awaiting review after 3 months. @dstftw @remitamine @phihag

@woodytec
Copy link

woodytec commented Aug 5, 2020

Any news about merging this? Thanks! @dstftw @remitamine @phihag

Fixed news videos by implementing new window.__INITIAL_DATA__ data, and also fixed Morph videos.
@GreenReaper
Copy link

Is this still relevant given c32a059 ?

@georgeahill georgeahill mentioned this pull request Jan 18, 2021
@dirkf dirkf mentioned this pull request Feb 25, 2021
11 tasks
@dirkf
Copy link
Contributor

dirkf commented Feb 9, 2022

I believe this is now superseded.

@dirkf dirkf closed this Feb 9, 2022
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.

None yet

5 participants