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

[Housekeeping] Pass the current action when calling the yt-dlp runner #514

Merged
merged 5 commits into from
Dec 13, 2024

Conversation

kieraneglin
Copy link
Owner

What's new?

  • All calls to the yt-dlp runner now pass an action argument where the action is typically the current method name
    • This resolves a long-standing issue with the way yt-dlp is mocked in tests. Since there's only one "entrypoint" into yt-dlp, it made mocking with Mox extremely error prone and fragile since mocks would have to either abuse the fact that some yt-dlp calls had different arity or pattern match against things like the output template. The new approach allows me to match based on the action being taken (a proxy for a method name in a more typical setup) and now I can be sure that I'm mocking the correct thing and I don't have to resort to sketchy bodges. Again, this isn't typically an issue but it was in this case since the run method is essentially an opaque entrypoint into a whole host of other functions.
      I was resistant to this change for a while since it places a pure testing concern into my app's code, but I'm okay with it after seeing the new API it presents. Besides, its surface area is small since I have a delegation layer for these calls
    • Adds logging of the current action
    • Updates a lot of tests
    • Pre-req for [Triage] Unable to download age restricted videos, using cookies #509

What's changed?

N/A

What's fixed?

N/A

Any other comments?

N/A

@kieraneglin kieraneglin merged commit 023f449 into master Dec 13, 2024
1 check passed
@kieraneglin kieraneglin deleted the ke/pass-action-to-yt-dlp-runner branch December 13, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant