Refactor artist to make use of Song.save_lyrics()#84
Conversation
|
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 |
|
This is great work, Vito. Thank you! I have a busy few days, but I'll try to get to the PR soon. |
There was a problem hiding this comment.
I like all of the changes you've made.
One request --
I'd like for the Artist.save_lyrics() function to result in just a single file, not one file per song. Can you modify the code to export and save a single JSON object? Along with the songs field, the JSON object created by the Artist.save_lyrics() method should also contain fields for the Artist object properties (name and image_url).
When the extension is txt, perhaps the user could be given the option to export a single text file with all lyrics concatenated together or as separate files.
artist.save_lyrics(extension='txt', multiple_files=True)Default value for multiple_files should be False. The JSON save option could use the multiple_files flag as well.
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).
Does that all make sense? Thanks for these contributions, they're great!
|
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. 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. |
|
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 Make sense? Necessary requested changes for tests:
|
|
I updated the tests to remove the multiple files |
|
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. |
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