search

jmbannon / ytdl-sub

Lightweight tool to automate downloading and metadata generation with yt-dlp
https://ytdl-sub.readthedocs.io
GNU General Public License v3.0
1.74k stars 69 forks source link

[BACKEND] Update yt-dlp, move to pyproject.toml #965

Closed Noodlez1232 closed 5 months ago

Noodlez1232 commented 5 months ago

This also includes some dependency updates. I would really appreciate a review of them.

I don't know much about how the versioning in python works (I am not really a Python developer), but I know there's instead a type of versioning that works more like this: yt-dlp>=2023.05.03 where any version newer than that version is compatible. I was wondering if maybe it should move to this? It might break the project in the future, but it'd be a loud break, meaning it'd be easy to catch and fix. There's also a type called "Compatible release" that works like this: sphinx~=7.2.6 that might be useful for some of the dependencies that are more important to pin.

Fixes #961

Noodlez1232 commented 5 months ago

To clarify, I did not test or do anything that has to do with deps yet, I'm waiting for you to take a look and see what you think, and then I'll continue with that part. It's a bit difficult to test with deps on NixOS, so I'm waiting for that.

jmbannon commented 5 months ago

This looks great! With yt-dlp I've been meaning to set up auto-deps which creates a PR automatically with the new version. The other deps should be safe to allow minor versions

Noodlez1232 commented 5 months ago

Looking through the test failures, it seems there may be some sort of dependency failure between pytest-rerunfailures and ytdl-sub since they require different versions of the same package. I'll look into this as well. For now, I'll keep messing with it and see what I can do in terms of dependency resolution and all that. I'll make sure I tighten up the dependency on yt-dlp as well. I'll keep it pinned.

Noodlez1232 commented 5 months ago

~~Btw looking into the problem of the PR, it seems that dependencies will be able to be dynamically generated soon (using setuptools). It's in beta right now. It'll dynamically pull that info from a requirements.txt type file which'd be easy to sed in order to auto-generate a yt-dlp update pull using CI. Just make it always line 0 or something and then you'd be able to just generate it on-the-fly. I don't know how it interacts with the existing dependencies specifier in [project], whether it just adds to it, has precedence, or overwrites those values completely. Of course, it's in beta, so probably not important right now, but maybe one day.~~

EDIT: Nvm I just learned about Dependabot

jmbannon commented 5 months ago

Sorry for the long afk -- you can run make lint to get linter to pass. I will have time later this week if you're not around to get this over the finish line

Noodlez1232 commented 5 months ago

I'm hoping that worked... I had some trouble getting the environment up, so idk if it worked.

jmbannon commented 5 months ago

@Noodlez1232 Going to keep the lint deps stationary to avoid having this PR alter anything there. Everything else looks good! Once the build tests pass, I will merge. The other tests are failing most likely from stale hashes -- something I need to update in a separate PR