tamland / python-tidal

Python API for TIDAL music streaming service
GNU Lesser General Public License v3.0
413 stars 110 forks source link

Downgrade minimum required version of requests #161

Closed JoshMock closed 1 year ago

JoshMock commented 1 year ago

Last month, in https://github.com/tamland/python-tidal/commit/dc44738c9a2425ac6a2387cd5c02cad3bd5c68b2, @arusahni explicitly set the requests version to ^2.31.0, the latest stable release. Obviously latest is ideal, especially when it's a security release. However on Arch Linux right now, the python-tidalapi AUR package depends on python-requests installed from the Arch extra repository, which is currently at 2.28.2 (and has been flagged out of date for 3 months, unfortunately). Because of that, I currently can't use mopidy-tidal installed from AUR either.

Since Python dependency management is a known pain in environments where a project is not sandboxed in a container or virtualenv, being slightly more permissive about dependency versions is ideal for ensuring compatibility is a bit more forgiving.

2e0byo commented 1 year ago

I have no problem with this. (@tehkillerbee ?)

Longer term I'd like to swap out requests for httpx, but there are more important fish to fry.

I don't understand your devdeps target: poetry installs development dependencies by default. (Indeed all groups not marked optional). I'm ambiguous about making test depend on install: I can see the logic, but I've never seen a makefile that did that and I think we should just error if the user hasn't run install. (Inter alia, modifying the venv and then running the test suite without updating pyproject.toml is a legitimate, albeit rare, thing to do, and I don't see why the test target should have non-obvious side effects like changing the venv: testing should be idempotent.) I'm open to persuasion, but as it stands I'd just revert that commit I think. (If you really think it's a problem you could add a warning, perhaps with grep on poetry install --sync --dry-run.)

2e0byo commented 1 year ago

Now I've pushed comment I guess the point was just to install the venv which I don't actually provide in the Makefile. In that case I think something like

develop:
    poetry install

would be better. The install target was by analogy with sudo make install. I guess help should be clearer too. Thoughts?

Oh, and thanks for nice clear commit messages!

tehkillerbee commented 1 year ago

@2e0byo I agree, I think we can safely downgrade.

Regarding the makefile, I think it could make sense as you suggest, wrapping poetry install as a target in the makefile. Then it is probably a bit easier for people new to poetry.

JoshMock commented 1 year ago

I don't understand your devdeps target

Yeah that was silly, in retrospect. There are so many ways to manage Python deps, I forget how poetry's particular flavor of magic works. :laughing: I dropped that commit and pushed with just the change to requests.

2e0byo commented 1 year ago

Python deps are a mess. I still prefer them to JS, but that's not saying much.

Thanks for the PR, I'll merge this. @tehkillerbee I'll add a develop target and update the help.