-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fix lrclib lyrics #5406
base: master
Are you sure you want to change the base?
Fix lrclib lyrics #5406
Conversation
d4bed72
to
829192d
Compare
cb8929f
to
c2807f0
Compare
The build on I will address this in a separate PR and rebase this one accordingly once the fix is merged. Note: this issue popped up now because I added a new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a much better implementation now, well done. I especially like that you've managed to remove the test resources -- I wasn't expecting 2000 lines of lyrics in there.
…5407) See my comment under #5406 for context > The build on win32 is failing to install reflink because it's [only supported until Python 3.7](https://gitlab.com/rubdos/pyreflink/-/blob/master/setup.py?ref_type=heads). > > I will address this in a separate PR and rebase this one accordingly once the fix is merged. > > Note: this issue popped up now because I added a new requests-mock dependency which invalidated cached dependencies.
c2807f0
to
64c0439
Compare
dc02c94
to
622ed3c
Compare
622ed3c
to
3eb04ba
Compare
1ff3ff2
to
4d481d7
Compare
9518b5d
to
27453e3
Compare
Hey, LRCLIB author here 👋. It would be great if you could make the Also, the If you have any ideas for further improvements to LRCLIB's API, feel free to let me know! |
Hi @tranxuanthang, thanks for popping in! Absolutely, that's no problem at all. This should make most lyrics queries even speedier on our side. Now that we're on this topic, I think it may be a good idea to also add caching: for example, if we're getting lyrics for two separate files
Ideally we should only ask for @tranxuanthang thanks for a reliable and performant API! |
Create 'helpers.ConfigMixin' which sets up testing configuration. This is helpful for tests (e.g. test_lyrics.py) that only need the configuration and do not require temp dir. (#5102) Refactor lyrics tests to fix the issue global beets config issue. Additionally, add 'integration_test' mark that can be used to mark tests that should only run once a week.
Improve requests performance with requests.Session which uses connection pooling for repeated requests to the same host. Additionally, this centralizes request configuration, making sure that we use the same timeout and provide beets user agent for all requests.
Due to request error handling this logic will only run for successful requests, so we can safely assume the html to be a string.
Tidy up 'Google.is_page_candidate' method and remove 'Google.sluggify' method which was a duplicate of 'slug'. Since 'GeniusFetchTest' only tested whether the artist name is cleaned up (the rest of the functionality is patched), remove it and move its test cases to the 'test_slug' test.
0012d50
to
2602482
Compare
2602482
to
16cfd91
Compare
Fixes #5102
Fixes #5133
LRCLib
Adjust the base URL to perform a
/search
instead of attempting to/get
specific lyricswhere we're unlikely to find anything for some specific combination of album, artist and
title fields.
Since we receive an array of matching lyrics candidates, rank them by
and pick the best match.
Additionally, I did a small refactor of lyrics integration tests and removed some unused lyrics files.