-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Added Support for downloads from text files and also added a lyrics provider. #859
Conversation
s1as3r
commented
Sep 30, 2020
•
edited
Loading
edited
- Added support for downloads from a text file.
- Now also adds lyrics to the downloaded song.
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.
Please look through the CONTRIBUTING.md
If you'd have let me know when you were in the dev phase, most of the issues could have been sorted out b4 you put in so much effort.
I'll ask the community for feedback on lyrics. and let you know if the feature has to be added within the week. You can delay changes until then if you wish or start working on them when free.
Well @phcreery is working on a better cli. So tqdm might be removed. Look into his work at his fork if you wish. His stuff will take some time though. |
Instead of using |
Cool, so we using genius or music match? |
Genius, because it doesn't have a limit on its api tokens. |
So we aren't scrapping music match with re? That would be so much better. Spotify and music match have a deal so almost all spotify songs have time synched lyrics on musicmatch. |
Well at least for now, not a lot of people are voting on features. That's sad. Yours has among the most traction. If it gets through (at least 10 upvotes) genius is cool. We could add better providers later. |
Any idea on how to get more people to vote? |
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.
I'll be going through your code properly (offline, in an editor) in the next few days. To catch all the not so obvious changes needed.
Please 'resolve conversation' my comments when they're fixed/no longer relevant. |
I'll make all the changes within a day or two. I'm kinda busy rn. |
Sure, I'm actually very busy rn. Take your time, I may not be able to look up your changes for like a week. |
Sorry to keep you waiting. Ik its frustrating to wait. I'll test it and merge it by today. And please change the destination branch to |
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.
It works, It some of the very best code I've seen to date.
Excellent!
Just the 1 or 2 single-line edits in provider.py and this is good to go. I won't be reviewing this again. You have the all clear.
Great job.
print('Fetching songs from %s...' % request) | ||
songObjList = [] | ||
|
||
with open(request, 'r') as songFile: |
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.
CliArgs
is a list, it's much more simpler to add the links to CliArgs
- that way, the .txt
file can have song, playlist and album links and still work.
From a maintenance standpoint, fewer lines of code is always better.
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.
But, there's a catch, if the .txt file is the last argument, spotdl will just skip the songs. And it won't be able to take advantage of parallel downloads.
And, is there any way maintainers can make changes instead of asking contributors to make nitpicky changes each time? |
Amazing job, Mr. Shafiq. Lots of people waiting for this change. |