inflatablefriends / lastfm

Portable .Net library for Last.fm
Other
100 stars 62 forks source link

Add optional parameters to the GetRecentTracks request #144

Closed L-Dogg closed 5 years ago

L-Dogg commented 5 years ago

Contiunation of #123 Fixing #113. For now one unit test fails because of some response encoding issues - I'll try to fix that later today.

L-Dogg commented 5 years ago

I don't know if the issue related to the deserialization or the file I'm using for testing the extended response, which contains polish diacritics.

klinge commented 5 years ago

Ah, character encoding. One of the really messy things in programming!! :) Maybe @rikkit could have some input on this!

In the GetRecentTracksCommand.cs I think you are still missing setting the extended parameter in the SetParameters() method. I think something like this would be required to pass the extended parameter to the url that is used.

           if (To.HasValue)
            {
                Parameters.Add("to", From.Value.AsUnixTime().ToString());
            }
            if (Extended.HasValue)
            {
                Parameters.Add("extended", Extended.ToString());
            }

A tip would be to make an integration test using the new parameters to, from and extended and verify that you get the response you expect from LastFM.

klinge commented 5 years ago

I was thinking a bit about this. I tested calling ArtistGetInfo with "lang=sv" to get swedish special characters (I'm Swedish) and this works without any issues. Since this pull request is for adding new parameters to GetRecentTracks - why not make a unit test without the polish characters and then create a new bug/issue on character handling?

L-Dogg commented 5 years ago

@klinge I found some time to "fix" the unit test, I'll create an issue later today. Meanwhile, can you check this pull requeset once again?

klinge commented 5 years ago

This looks good to me. @rikkit will check it before merging too.