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

[core] Fix simulate changing format selection #9862

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

seproDev
Copy link
Collaborator

@seproDev seproDev commented May 5, 2024

IMPORTANT: PRs without the template will be CLOSED

Description of your pull request and other information

Fixes #9843

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?

@seproDev seproDev added the bug Bug that is not site-specific label May 5, 2024
While `can_merge` currently always returns `True`, I think it's better to mock this to not call `FFmpegMergerPP`
@seproDev
Copy link
Collaborator Author

seproDev commented May 5, 2024

Is changing the signature of _default_format_spec by dropping the download parameter as done in 70ca5fb okay?
This function isn't part of the public API, so imo. it should be fine. Interested to hear what everyone else thinks.

@pukkandan
Copy link
Member

Personally, I'd have added a deprecation warning anyway, coz I'm conservative with this stuff. But it's not really needed. Your patch looks fine.

But to clarify, my comment was not demanding we do it this way, but simply to give my opinion. We should try to have identical behavior with ytdl on this. So let's coordinate with dirkf on which is better.

@seproDev
Copy link
Collaborator Author

seproDev commented May 5, 2024

I agree with you in that I think it makes sense to change the downloaded=False behaviour in one swoop. That's why I added it to the PR.

@bashonly
Copy link
Member

bashonly commented May 5, 2024

I also think it's better to have download=False behave as if simulate=True. @dirkf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug that is not site-specific
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

--simulate doesn't accurately simulate downloading under certain conditions
3 participants