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

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

Merged
merged 2 commits into from
Jul 20, 2024

Conversation

jerz4lunch
Copy link
Contributor

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have read the CORE VALUES document
  • I have added tests to cover my changes
  • All new and existing tests passed

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

@Cornelius-Figgle
Copy link

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
Copy link
Contributor

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
Copy link

How come?

@aminvakil
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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

@Cornelius-Figgle
Copy link

No worries :)

@jerz4lunch
Copy link
Contributor Author

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 jerz4lunch closed this Jun 27, 2024
@jerz4lunch jerz4lunch reopened this Jun 27, 2024
Seems to have fixed the update of syncedlyrics to 1.0.0
@jerz4lunch
Copy link
Contributor Author

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
Copy link

I am not sure how to test the lyrics functionality

Install pytest and run the tests I think.

@jerz4lunch
Copy link
Contributor Author

jerz4lunch commented Jun 28, 2024

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
Copy link
Contributor

pekkarr commented Jun 29, 2024

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

cc @aminvakil

@aminvakil
Copy link
Contributor

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

@Cornelius-Figgle
Copy link

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

@xnetcat xnetcat changed the base branch from master to dev July 20, 2024 13:11
@xnetcat xnetcat merged commit a5453af into spotDL:dev Jul 20, 2024
@xnetcat xnetcat mentioned this pull request Jul 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants