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

Clearly mark dependent InfoExtractors #7483

Closed
6 of 9 tasks
trainman261 opened this issue Jul 1, 2023 · 4 comments
Closed
6 of 9 tasks

Clearly mark dependent InfoExtractors #7483

trainman261 opened this issue Jul 1, 2023 · 4 comments

Comments

@trainman261
Copy link
Contributor

DO NOT REMOVE OR SKIP THE ISSUE TEMPLATE

  • I understand that I will be blocked if I intentionally remove or skip any mandatory* field

Checklist

Provide a description that is worded well enough to be understood

A number of InfoExtractors are reliant on other InfoExtractors. Sometimes, it's necessary to make adjustments to IEs that other IEs rely on. Problem is, this isn't generally a formal dependency, rather, another IE will hand back a link that is to be handled by a different IE.
The problem arises when changes are necessary to an IE that other IE's rely on: whereas there are tests for that specific IE, that hardly covers what should be tested to avoid regressions - it's not easy at all to figure out if you're breaking another IE.
Example: the CBCPlayerIE relies on the ThePlatformIE to extract information, as do many other IE's. If a change in the ThePlatformIE is necessary for the CBCPlayerIE, it's really hard to figure out what other IE's might get broken by that change.
Would it be possible to somehow get the IE's linked? The goal being, that when making changes to the IE, that it's easy to figure out which other IE's might be affected and ideally systematically test them as well. That way, it would easier to ensure that no regressions occur.
Just a thought, maybe checking if 'ie_key': 'ThePlatform' is included in the return statement from _real_extract?
Or maybe an organizational measure - I'm honestly not sure what the best solution here would be, but I'm pretty sure there's room for improvement over the current situation.

Provide verbose output that clearly demonstrates the problem

  • Run your yt-dlp command with -vU flag added (yt-dlp -vU <your command line>)
  • If using API, add 'verbose': True to YoutubeDL params instead
  • Copy the WHOLE output (starting with [debug] Command-line config) and insert it below

Complete Verbose Output

No response

@trainman261 trainman261 added enhancement New feature or request triage Untriaged issue labels Jul 1, 2023
@trainman261 trainman261 mentioned this issue Jul 1, 2023
9 tasks
@bashonly
Copy link
Member

bashonly commented Jul 1, 2023

grep "ThePlatform" yt_dlp/extractor/*.py etc has worked for me

@pukkandan pukkandan added question Question and removed enhancement New feature or request triage Untriaged issue labels Jul 2, 2023
@trainman261
Copy link
Contributor Author

I guess that's always an option... I was figuring something a little more exact and/or systematic. That would also catch comments that mention ThePlatform (maybe because a site used to use ThePlatform but it doesn't anymore). Especially with infoextractors that have somewhat common names, it could catch more than it should.

@trainman261
Copy link
Contributor Author

Now that I've been trying to fix up tests a bit, I'm wondering if the test system for the IEs could use a revamp in general. Another difficulty I've run into is the skipping tag. There's only a text option, but no option to specify for example to which countries the video is geo-restricted from or to. These then get skipped even if they're available in the location the dev is testing from. Maybe some expansion of the skip tag to more explicitly state which countries work, which don't, and which aren't clear?
Mind you, again, maybe I'm missing something or overthinking things. Guess the question is: would there be interest in this? It just seems like there's quite a bit of room for improvement. If I get around to it, I would be ready to also put in some work for this.

@pukkandan
Copy link
Member

pukkandan commented Apr 14, 2024

Maybe some expansion of the skip tag to more explicitly state which countries work, which don't, and which aren't clear?

The test runner would have to check dev's IP and country for it to work. It's out of scope imo. Download tests are run manually anyway. Just commenting out the skip is fine, no? Also, if the whole IE is geo-restricted, it's not recommended to add skip in the first place.


As for OP, we try to import the IE instead of hardcoding key for new extractors - then grepping ThePlatformIE or using an LSP will be reliable. But it's not feasible to do that for all legacy code, so you'd still have to search the string...


Closing this since I don't see any concrete path to improve the situation. Feel free to continue discussion if anyone have any ideas

@pukkandan pukkandan closed this as not planned Won't fix, can't repro, duplicate, stale Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants