search

johnwmillr / LyricsGenius

Download song lyrics and metadata from Genius.com 🎶🎤
http://www.johnwmillr.com/scraping-genius-lyrics/
MIT License
899 stars 159 forks source link

Refactor artist to make use of Song.save_lyrics() #84

Closed VitoMinheere closed 5 years ago

VitoMinheere commented 5 years ago

Resolves #80

Refactor artist to make use of Song.save_lyrics() for all the songs in the artist object

Created to_dict function on Song to create the object for the json extension option.

Set absolute imports

Fixed some typos

Changed format_ to extension as not to confuse with Python format

VitoMinheere commented 5 years ago

Updated the tests so that they will all complete.

Because each song for the artist is saved separately the filename parameter won't work, the files will all get the same name. Now each song is save in a different file. A check could be added that if filename is set, all the lyrics will be written to 1 file

johnwmillr commented 5 years ago

This is great work, Vito. Thank you! I have a busy few days, but I'll try to get to the PR soon.

VitoMinheere commented 5 years ago

Different extensions would also require adding more options to the main.py. This refactor will be a bit bigger than expected.

I am not sure if using the song.save_lyrics would be the best way to proceed. That function will now write directly to a file. Perhaps 2 write_lyrics functions are necessary but the amount of duplicate code will need to be minimized.

Maybe it would be better to split all the functions apart. Both Song and Artist would then have functions for creating a json object and for saving the lyrics/songs.

johnwmillr commented 5 years ago

I realize my PR review was a bit disjointed. If you don't mind tackling the updates to the unit tests I requested, I'd be happy to merge the PR as is. Then, I (or both of us) can get started on some of the necessary modifications for my other requests.

Why can't you just use your Song.to_dict() method to create the JSON object within Artist.save_lyrics()?

Make sense?

Necessary requested changes for tests:

Also, because the default save filenames have changed, the test functions aren't deleting the temporary lyrics save files properly anymore. The lyrics files saved during tests should get deleted once the testing completes (while leaving any of the user's saved lyrics files in place, of course).

VitoMinheere commented 5 years ago

I updated the tests to remove the multiple files

johnwmillr commented 5 years ago

Good work! I'm happy with the changes you made and will merge the PR. I'll start work on a new branch implementing the lyrics saving options you described above. You're more than welcome to contribute.