GyrosOfWar / youtube-dl-rs

A youtube-dl wrapper for Rust
132 stars 40 forks source link

Remove support for youtube-dl, add helper for downloading yt-dlp #28

Closed GyrosOfWar closed 1 year ago

GyrosOfWar commented 2 years ago

Closes #6

GyrosOfWar commented 2 years ago

@twiclo and @ticky, can I maybe ask you for feedback on this? I feel like Cargo features are a bit of an awkward fit for this, so I removed the yt-dlp feature and made it into a runtime switch (use_yt_dlp() on YoutubeDl).

twiclo commented 2 years ago

I would ask @ticky about the whole runtime switch thing. I'm not very familiar with them and he might be able to provide a better answer. I will say that if I were you I would just drop support for vanilla youtube_dl. While it's still supported the owners have said they're only going to support python2 moving forward and I can't remember the last time youtube_dl actually worked

ticky commented 2 years ago

It's she, FYI! ☺️


Honestly I think in your situation I'd keep it a Cargo feature flag but rather than having a youtube-dl and yt-dlp features, I'd just make the default features empty and switch off the incompatible flags if the other feature is enabled. Much as I realise it makes it somewhat harder to switch, I doubt it'll be the common case to do so, given the maintenance state of OG youtube-dl. I might even go as far as making vanilla youtube-dl support the opt-in.

If you're set on it being runtime-configurable, my suggestion would be either to make it two different structs (one for YoutubeDl and one for YtDlp), or make YoutubeDl accept a generic argument to toggle the implementation.

ticky commented 2 years ago

To be clear what I mean by my suggestion is effectively, anywhere you're currently depending on the yt-dlp feature, make it depend instead on the YouTube-dl feature being off; cfg can be used like this;

#[cfg(not(feature = "youtube-dl"))]
GyrosOfWar commented 2 years ago

Alright, thanks for the input. I'll keep the compile-time flag and make yt-dlp the default.