-
Notifications
You must be signed in to change notification settings - Fork 58
update lyrics library #92
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
Conversation
went from - lyricwikia (not supported) to - lyriq (supported)
pwittchen
left a comment
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.
Thank you for the PR!
In general, it looks fine, but you forgot to remove lyricwikia dependency from the requirements.txt and add there new lyriq dependency. Moreover, you can also remove section related to lyricwikia from the README.md, because it will be no longer needed after this change. I'm also wondering if these logging import and instructions are needed here.
spotifycli/spotifycli.py
Outdated
| lyrics = lyriq.get_lyrics(title, artist) | ||
|
|
||
| if lyrics is None: | ||
| # Lyrics not found |
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 comment is not necessary
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.
Got it
| import textwrap | ||
| import datetime | ||
| from subprocess import Popen, PIPE | ||
| import logging |
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.
is this import needed?
| import lyriq | ||
|
|
||
| import lyricwikia | ||
| logging.getLogger("lyriq.lyriq").setLevel(logging.CRITICAL) |
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.
is this call neded?
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.
Without it the lyriq library will print a 404 error when it does not found the song specified on it's database. So to make the logging more clean i added that call.
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.
ok
|
I have push the changes to the |
went from using
to