Conversation
There was a problem hiding this comment.
Thank you for your work on this PR! I've left a few comments with some suggestions for improvements in the code.
Additionally, I tested a complex case with the following URL: Attack Attack - I Swear I'll Change, and encountered an error. Please have a look into this issue to ensure proper handling of such cases.
Error is:
I tried the following fields in search request:
string artistToSearch = "Attack Attack!";
string songToSearch = "I Swear I'll Change";
Also, in the Readme.md, I suggest replacing, if you don't mind:
LyricsFreak (Coming soon 🚧. Issue #38)
with: LyricsFreak (added by @ajay201402)
| @@ -0,0 +1,150 @@ | |||
| using Genius.Models.Song; | |||
There was a problem hiding this comment.
Pleasse sort and remove unused usings
|
|
||
| if (WebClient == null || Parser == null) | ||
| { | ||
| _logger?.LogWarning($"LyricFind. Please set up WebClient and Parser first"); |
There was a problem hiding this comment.
There is a copy-paste issue in the log messages. For instance, "LyricFind" appears incorrectly here and in other places in this file.
|
|
||
| namespace LyricsScraperNET.Providers.LyricsFreak | ||
| { | ||
| internal sealed class LyricsFreakUriConverter : IExternalUriConverter |
There was a problem hiding this comment.
Plaese Add Unit Tests for LyricsFreakUriConverter similar to other converters. You can refer to test cases in AZLyricsUriConverterTests. Ensure that the information is correctly fetched from the specified address when writing the tests.
| return lyricsScraperClient; | ||
| } | ||
|
|
||
| public static ILyricsScraperClient WithLyricsFreak(this ILyricsScraperClient lyricsScraperClient) |
There was a problem hiding this comment.
Please add a test in the LyricsScraperClientExtensionsTest like: LyricsScraperClient_WithLyricsFreak_ReturnsIsEnabled
| return result; | ||
| } | ||
|
|
||
| public static string СonvertSpaceToPlusFormat(this string input, bool removeProhibitedSymbols = false) |
There was a problem hiding this comment.
Please add a couple unit tests in the:
StringExtensionsTest
Take a look at the ases in: СonvertToDashedFormat_MultipleInputs_ShouldBeParse
skuill
added
enhancement
|
Hi @skuill I will take this up and reply soon. Thanks for the comments |
|
@skuill All the review comments are fixed. Please check |
skuill
changed the title
skuill
changed the title
|
Hi @ajay201402 ! Thank you for your PR and contribution and fixing all the PR comments! I'll merge your PR and do a final review of the package before publishing it. I'll let you know when the package is published. |
2 participants
Changes done:
LyricsScraperNet project
LyricsScraperNet.Client Project
Tests.