Skip to content

[core] Fix simulate changing format selection #9862

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Jul 8, 2024
Merged

[core] Fix simulate changing format selection #9862

merged 10 commits into from
Jul 8, 2024

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
seproDev added 4 commits May 5, 2024 12:03
@seproDev
Also mock can_merge
0795c53 
While `can_merge` currently always returns `True`, I think it's better to mock this to not call `FFmpegMergerPP`
@seproDev
Adjust patch target
de5634e
@seproDev
Also adjust behavior of download=False
70ca5fb
@seproDev
Revert "Adjust patch target"
db13541 
This reverts commit de5634e.
@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

bashonly
bashonly reviewed Jul 7, 2024
View reviewed changes
@seproDev @bashonly
Update README.md
b99d103 
Co-authored-by: bashonly <88596187+bashonly@users.noreply.github.com>
bashonly
bashonly approved these changes Jul 7, 2024
View reviewed changes
@seproDev @bashonly
Fix typo
fb9c2e8 
Co-authored-by: bashonly <88596187+bashonly@users.noreply.github.com>
@seproDev seproDev merged commit 0b570f2 into yt-dlp:master Jul 8, 2024
15 checks passed
@seproDev seproDev deleted the fix-simulate-behaviour branch July 8, 2024 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Grub4K Grub4K Grub4K approved these changes

@bashonly bashonly bashonly approved these changes

Assignees
No one assigned
Labels
bug Bug that is not site-specific
Projects
Release Tasks
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

--simulate doesn't accurately simulate downloading under certain conditions
4 participants
@seproDev @pukkandan @bashonly @Grub4K