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

[ie/NYTimes] - Extractor overhaul - Article, Cooking Recipes and Guides #9075

Merged
merged 26 commits into from Feb 5, 2024

Conversation

SirElderling
Copy link
Contributor

@SirElderling SirElderling commented Jan 26, 2024

IMPORTANT: PRs without the template will be CLOSED

Description of your pull request and other information

This is the first PR of my attempt to overhaul the existing, non working, extractor.
I'm pretty sure there is a lot to optimize on the code I'm submitting.

To try and facilitate the process, I decided to break all the adjustments I made in different PRs.

This first PR covers:

  • General Articles
  • Cooking Recipes
  • Cooking Guides

Fixes #8605 #2899

Future PRs will cover the extraction of:

  • Video Playlist (and single video)
  • Podcasts / Audio / Book Review
Template

Before submitting a pull request make sure you have:

In order to be accepted and merged into yt-dlp each piece of code must be in public domain or released under Unlicense. Check all of the following options that apply:

  • 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?

@bashonly bashonly added the site-bug Issue with a specific website label Jan 26, 2024
Copy link
Member

@bashonly bashonly left a comment

Choose a reason for hiding this comment

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

partial review

yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
'skip_download': True,
},
}, {
'url': 'http://www.nytimes.com/news/minute/2014/03/17/times-minute-whats-next-in-crimea/?_php=true&_type=blogs&_php=true&_type=blogs&_r=1',
Copy link
Member

Choose a reason for hiding this comment

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

This URL still works too, although it redirects to https://archive.nytimes.com/www.nytimes.com/news/minute/2014/03/17/times-minute-whats-next-in-crimea/?_php=true&_php=true&_r=1&_type=blogs&_type=blogs

We'll probably need to add a new NYTimesArchive extractor for this, which can be a separate PR. Just noting it here so that it's not forgotten

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll take this into consideration in the next PR.

yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
@bashonly bashonly added the pending-fixes PR has had changes requested label Jan 27, 2024
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
@bashonly
Copy link
Member

bashonly commented Feb 3, 2024

I made an edit to one of the review comments just now. Please make sure to work from the latest version of the comments before applying suggestions locally

@SirElderling
Copy link
Contributor Author

Side note: I'm using the 👍 in the comments to track the changes I applied in the local code.

@SirElderling
Copy link
Contributor Author

I made an edit to one of the review comments just now. Please make sure to work from the latest version of the comments before applying suggestions locally

I believe I got everything. Thank you.

yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
yt_dlp/extractor/nytimes.py Outdated Show resolved Hide resolved
@bashonly bashonly removed the pending-fixes PR has had changes requested label Feb 4, 2024
@bashonly bashonly self-assigned this Feb 4, 2024
@bashonly
Copy link
Member

bashonly commented Feb 4, 2024

It was trivial to (mostly) fix NYTimesIE with the code we already had, so I went ahead and did so. We are missing the timestamp/upload_date, though

Comment on lines 385 to 386
isLive
liveUrls
Copy link
Member

Choose a reason for hiding this comment

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

note: these may be useful in the future

@bashonly bashonly merged commit 07256b9 into yt-dlp:master Feb 5, 2024
6 checks passed
@SirElderling SirElderling deleted the nytimes branch February 5, 2024 13:44
FletcherD pushed a commit to FletcherD/yt-dlp that referenced this pull request Feb 14, 2024
aalsuwaidi pushed a commit to aalsuwaidi/yt-dlp that referenced this pull request Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
site-bug Issue with a specific website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NYTimes videos fail with "503: Backend is unhealthy" errors
2 participants