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

Various search improvements #375

Merged
merged 6 commits into from
Jul 4, 2024
Merged

Various search improvements #375

merged 6 commits into from
Jul 4, 2024

Conversation

Maritsu
Copy link
Contributor

@Maritsu Maritsu commented May 25, 2024

Closes #373, might close #374.

  • Uses YT Music for searching songs via ytmusicapi lib (note: the search filters by songs only!)
  • Removes "Lyrics" keyword from search query (avoids e.g. more popular songs with lyric videos or fan-made lyrics)
  • Makes use of Levenshtein edit distance (implemented in Levenshtein package) to get closest matching search result instead of the top one

Example of different result: Toby Fox - Death by Glamour

- add ytmusicapi as dependency
- modify dump_json() and find_and_download_songs() to use YT Music
Copy link

sentry-io bot commented May 25, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: spotify_dl/youtube.py

Function Unhandled Issue
find_and_download_songs IndexError: list index out of range spotify_dl.yo...
Event Count: 1

Did you find this useful? React with a 👍 or 👎

@Maritsu
Copy link
Contributor Author

Maritsu commented May 26, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: spotify_dl/youtube.py
Function Unhandled Issue
find_and_download_songs IndexError: list index out of range spotify_dl.yo...
Event Count: 1

Did you find this useful? React with a 👍 or 👎

I'm unable to open the linked issue, is this error my fault or has it been here previously?

@SathyaBhat
Copy link
Owner

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:
📄 File: spotify_dl/youtube.py
Function Unhandled Issue
find_and_download_songs IndexError: list index out of range spotify_dl.yo...
Event Count: 1
Did you find this useful? React with a 👍 or 👎

I'm unable to open the linked issue, is this error my fault or has it been here previously?

it's in my sentry account that is private

@Maritsu
Copy link
Contributor Author

Maritsu commented May 26, 2024

I see. I'm not sure I understand @sentry-io's message though - is the IndexError something I caused, or has it been in find_and_download_songs() before?

@Maritsu
Copy link
Contributor Author

Maritsu commented May 27, 2024

On another note, this PR doesn't seem to make that much of a difference in search result quality - I'll mark as draft and extend the functionality to select the closest match from the top few search results.

@Maritsu Maritsu marked this pull request as draft May 27, 2024 06:18
@Maritsu Maritsu marked this pull request as ready for review June 2, 2024 15:05
@Maritsu
Copy link
Contributor Author

Maritsu commented Jun 2, 2024

Should work somewhat better now. Unmarked as draft, ready for review!

@Maritsu Maritsu changed the title Improve search by using YT Music Various search improvements Jun 2, 2024
@SathyaBhat
Copy link
Owner

doesn't look like ytmusicapi supports Python 3.7.. need to remove support for it in the pipeline and add new versions

@Maritsu
Copy link
Contributor Author

Maritsu commented Jun 3, 2024

yup, just noticed it requires Python 3.8 or newer. should I update the pipeline then?

@SathyaBhat
Copy link
Owner

SathyaBhat commented Jun 4, 2024 via email

@Maritsu
Copy link
Contributor Author

Maritsu commented Jun 5, 2024

sure, i'll try as soon as i can

@Maritsu
Copy link
Contributor Author

Maritsu commented Jun 5, 2024

update done! i changed the version range to 3.8-3.12

@Maritsu
Copy link
Contributor Author

Maritsu commented Jun 9, 2024

didn't notice that, thank you for correcting!

Copy link

@tposejank tposejank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Makes searching for song videos specific and accurate.
Using YouTube Music is really smart for this case. My original request was to change lyrics to "Official Audio" but this is way better.

This will close #374 because it's the same issue, not downloading correct videos etc, it's explained there.

@tposejank
Copy link

Another comment I'd like to leave here is some artists do like to publish their own Official Audio as a video and sometimes their YouTube release won't show up first in the search results, but that issue could be small and not matter a lot

@SathyaBhat SathyaBhat merged commit 934b6ee into SathyaBhat:master Jul 4, 2024
6 checks passed
@Maritsu Maritsu deleted the improve-search branch July 5, 2024 11:43
@Maritsu Maritsu mentioned this pull request Jul 11, 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.

Change the query to search songs with YouTube Wrong songs downloaded
3 participants