Closed brenol closed 5 years ago
@brenol - Thanks for the PR. Just a few points,
Please send a squashed commit with the changes mentioned in 2.
Thank you.
I will add a test for lyrics.go after your PR is merged along with removing log.fatal lines (thanks for pointing that out, I had missed this).
Regards
Oh! Sorry, I did not know that NewDocument was being deprecated since I've been using it for a long time for some quick stuff.
I'll modify the PR and send it over with everything correctly. Thanks for pointing that out!
OK, think I squashed correctly. Really really not used to doing rebases and push -force.
Thanks!
@brenol LGTM, have merged the changes! Thanks for your contribution. :+1:
Hi! I made a few small changes to improve LyricApi.
I think the idea is very nice but there are a few other cases that I did not handle in this pull-request:
There are many log.Fatal lines all over the library - those should be removed because they would make the application to abort completely; making lyric-api only useful for those that wouldn't use in long-running programs. I did not make the change to all those log.Fatal lines because that would break the API compatibility (returning an error).
I made a few small changes in how goquery was being used just to remove a few lines of code
I fixed the Genius implementation as fetch was always looking for "John Lennon - Imagine"
The idea is very nice though; hope I helped to improve it a little bit.