Skip to content

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

Merged
johnwmillr merged 7 commits into
johnwmillr:masterfrom
VitoMinheere:save_lyrics_refactor
Feb 14, 2019
Merged

Refactor artist to make use of Song.save_lyrics()#84
johnwmillr merged 7 commits into
johnwmillr:masterfrom
VitoMinheere:save_lyrics_refactor

Conversation

@VitoMinheere
Copy link
Copy Markdown
Contributor

@VitoMinheere VitoMinheere commented Feb 7, 2019

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Owner

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

@johnwmillr johnwmillr self-requested a review February 9, 2019 18:43
Copy link
Copy Markdown
Owner

@johnwmillr johnwmillr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@VitoMinheere
Copy link
Copy Markdown
Contributor Author

VitoMinheere commented Feb 12, 2019

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
Copy link
Copy Markdown
Owner

johnwmillr commented Feb 12, 2019

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
Copy link
Copy Markdown
Contributor Author

I updated the tests to remove the multiple files

@johnwmillr
Copy link
Copy Markdown
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants