spotDL / spotify-downloader

Download your Spotify playlists and songs along with album art and metadata (from YouTube if a match is found).
https://spotdl.readthedocs.io/en/latest/
MIT License
15.55k stars 1.48k forks source link

Update lrc.py for updated function in `python-syncedlyrics` #2120

Open jerz4lunch opened 2 weeks ago

jerz4lunch commented 2 weeks ago

See syncedlyrics commit: here

Change function call from is_lrc_valid to has_translation

I changed the import of is_lrc_valid to has_translation and changed function call in the same way.

Related Issue

It should solve the problem where the program wouldn't start since the function name was changed in required package python-syncedlyrics

How Has This Been Tested?

I tested this, but poetry brings in older version of syncedlyrics than the one in the AUR which doesn't have relevant change, so I am not sure it this will actually work. I am not sure how to test it with the correct version of the dependency. If someone could guide me on how to use the correct version with poetry, I would greatly appreciate it. It looks like it should work given the commit in the project.

Screenshots (if appropriate)

Types of Changes

Checklist

This is my first time creating a pull request, so any guidance is appreciated.

Cornelius-Figgle commented 1 week ago

If someone could guide me on how to use the correct version with poetry, I would greatly appreciate it.

Not sure if this is what you mean, but you can change the version set in pyproject.toml. See here for more details. I believe that as syncedlyrics has had a major version change, poetry will not automatically update it. In your case, changing the version of syncedlyrics to ^1.0.0 should be enough. You will then have to regenerate the lockfile and reinstall the environment.

The commands to do this are:

poetry add syncedlyrics@^1.0.0  # change version
poetry update                   # regenerate lockfile and reinstall
aminvakil commented 1 week ago

If someone could guide me on how to use the correct version with poetry, I would greatly appreciate it.

Not sure if this is what you mean, but you can change the version set in pyproject.toml. See here for more details. I believe that as syncedlyrics has had a major version change, poetry will not automatically update it. In your case, changing the version of syncedlyrics to ^1.0.0 should be enough. You will then have to regenerate the lockfile and reinstall the environment.

The commands to do this are:

poetry add syncedlyrics@^1.0.0  # change version
poetry update                   # regenerate lockfile and reinstall

This is just a temporary workaround and not a solution for this.

Cornelius-Figgle commented 1 week ago

How come?

aminvakil commented 1 week ago

How come?

I'm co-maintainer of https://aur.archlinux.org/packages/spotdl on AUR, and as python-syncedlyrics package has been upgraded to 1.0.0, you cannot depend on 0.10.0 of syncedlyrics anymore in my environment.

I don't mean to be demanding or anything by saying this, but it's not like everyone uses poetry to use this package.

Cornelius-Figgle commented 1 week ago

you cannot depend on 0.10.0 of syncedlyrics anymore

That is what @jerz4lunch was trying to fix no? Perhaps I misunderstand here, but my understanding was they had updated the SpotDL code to use the new version of syncedlyrics, but didn't no how to update the poetry details.

it's not like everyone uses poetry to use this package

Of course, but poetry is what is used to develop it, and some people do so all installation methods need to be updated to support 1.0.0

aminvakil commented 1 week ago

Oops! I thought what you meant was to stick to older version, sorry for inconvenience.

Cornelius-Figgle commented 1 week ago

No worries :)

jerz4lunch commented 1 week ago

Did what @Cornelius-Figgle suggested and now another function has to be updated. I would have looked into this before, but didn't know the proper steps to check. I'm closing this until I get everything working in my environment, and will submit another PR when that happens.

jerz4lunch commented 1 week ago

It seems to run just fine now with this latest update. The program runs. However, I am not sure how to test the lyrics functionality to make sure I didn't break something else. If I could get some help on that, I would greatly appreciate it!

Cornelius-Figgle commented 1 week ago

I am not sure how to test the lyrics functionality

Install pytest and run the tests I think.

jerz4lunch commented 1 week ago

I am not sure how to test the lyrics functionality

Install pytest and run the tests I think.

I got some errors. However, the test failures are not related to the code fix I made. So now what?

pekkarr commented 6 days ago

I backported this pr into the AUR package, and spotdl seems to work with it.

cc @aminvakil

aminvakil commented 6 days ago

I've also tested and it works fine: https://github.com/aminvakil/aur/actions/runs/9726681332/job/26845485832

Cornelius-Figgle commented 5 days ago

the test failures are not related to the code fix I made

I think you're alright then, not entirely sure though. I guess you need a contributor to merge this PR now