Prefer synced lyrics, options to allow only plain/synced #47
Conversation
syncedlyrics/providers/musixmatch.py
Outdated
| @@ -120,12 +128,13 @@ def get_lrc(self, search_term: str) -> Optional[str]: | |||
| return None | |||
| body = r.json()["message"]["body"] | |||
| tracks = body["track_list"] | |||
| cmp_key = lambda t: f"{t['track']['track_name']} {t['track']['artist_name']}" | |||
| def cmp_key(t): return f"{t['track']['track_name']} {t['track']['artist_name']}" | |||
There was a problem hiding this comment.
Any reason why you replaced lambda?
There was a problem hiding this comment.
Oh nothing in particular I think it got replaced automatically on save in vscode because pep8 conventions want this or something, I personally don't care, I can change it back
There was a problem hiding this comment.
Alright, the project follows formatting of black module. Can you also run poetry run black synceldyrics before merging.
syncedlyrics/providers/musixmatch.py
Outdated
| r = self._get( | ||
| "track.subtitle.get", | ||
| [("track_id", track_id), ("subtitle_format", "lrc")], | ||
| ) | ||
| print(self.lang) |
There was a problem hiding this comment.
Your're absolutely right, I accidentally left it in there from testing
| @@ -11,29 +11,42 @@ | |||
| from typing import List, Optional | |||
|
|
|||
| from .providers import Deezer, Lrclib, Musixmatch, NetEase, Megalobiz, Genius | |||
| from .utils import is_lrc_valid, save_lrc_file | |||
| from .utils import Lyrics, TargetType | |||
| from .providers.base import LRCProvider | |||
|
|
|||
| logger = logging.getLogger(__name__) | |||
|
|
|||
|
|
|||
| def search( | |||
| search_term: str, | |||
| allow_plain_format: bool = False, | |||
There was a problem hiding this comment.
I think introducing a class as the response of the search function will break the compatibility anyway. So allow plain would be no longer necessary instead we can replace it with plaintext_only.
There was a problem hiding this comment.
Well yeah, I was unsure how you feel about it, I also think it would be cleaner to return the class, rename the "allow_plaintext" to "plaintext_only", even though it breaks compatibility.
However in the current form I tried not to break compatibility and therefore made the search function still return a string (selecting from the Lyrics class according to the preference). If you want I can quickly change that.
There was a problem hiding this comment.
Sorry I thought you returned the Lyrics instance . In that case I think string is simpler and doesn't break anything so all good no change needed.
|
uhmmm... well that's unfortunate xD |
|
Seems Deezer doesn't work anymore lol. I wanted to comment out Deezer calls in search function and also put a help comment in readme but I don't know what happens if I change the main branch and your updates are based on previous version. I appreciate it if you can do it (also comment the Deezer test) so the tests pass and I will merge the changes. |
|
Ok, I used black to reformat, and changed the functions back to lambdas. Also commented out Deezer. Are we sure yet whether this broke permanently (due to an api change of theirs or sth) or if it's only temporarily down? If there's anything else you want to change before merging, let me know :) |
|
Great. Thanks again! |
--allow-plainoption still exists as an alias for the new (more concise)--plaintext-only--synced-onlyoption--enhanced: If no word-by-word sync is available, synced lyrics are used as a fallback. (Behaviour didn't change, but updated README for clarity)__str__method toLRCProvider, to avoid repeatedprovider.__class__.__name__()