Feature/adding User.getWeeklyChart-functions

[klinge]

I added support for the following missing api functions:

• user.getWeeklyChartList
• user.getWeeklyAlbumChart
• user.getWeeklyArtistChart
• user.getWeeklyTrackChart

In doing this I added a new Object called LastWeeklyCharList. Also had to make a small change in LastAlbum to handle that the artistname for some reason has the key "#text" in the returned json from getWeeklyAlbumChart.

All commands have unit tests and I also added integration tests.

[rikkit]

Ah, thanks @klinge! You're doing a lot for this project lately, it's much appreciated.

This is quite a hefty PR, so I'll take a little bit of time to review it - hopefully I can get to it soon though!

[klinge]

Thanks for the appreciation :) I'm just happy to help!

When you look at it you will see that the GetWeekly[Track|Album|Artist]Chart methods are all pretty similar. I thought about making something smart to avoid 3 similar commands. But since they call different api methods and return object of different types I ended up with 3 commands still.

I also tried to keep changes to "common" method as small as possible. I think the only "common" things i touched was

• Objects/LastAlbum.cs -> added a few ifs for handling name of the artist key varying between different json responses, also added parsing of ArtistMbid if present in the lastfm response
• Api/Helpers/PageResponse.cs -> added properties for From and To in the PageResponse

[klinge]

I've pushed the changes you requested after reviewing. But now the integration tests fail on the scrobbling functions (which are untouched in this PR):

• In the TrackScrobbleCommantTests ScrobblesSingle actual scrobble time if off by a few seconds to a few minutes from the expected value.

• In the TrackUpdateNowPlayingCommandTests the actual result contains ArtistImages where expected result is for it to be null.

Any ideas what could cause these tests failing?

[rikkit]

Hi @klinge thanks for the changes!

If I'm not wrong this becomes a bit complicated. The CreateSuccessResponse/CreateErrorResponse from PageResponse would need to be overridden in order to return a WeeklyChartResponse instead of a PageResponse. Also AddPageInfoFromJToken would need its own implementation to handle setting the To/From attributes.

You're right, I hadn't considered that. It would be good for convenience to have these values, but since the information is accessible by looking at the first and last items in the response object, I agree with the decision to leave that out for now.

Any ideas what could cause these tests failing?

My guess would be that the API behaviour has changed. As far as I know Last.fm don't publish when they're changing their API, so this is a problem that comes up occasionally. What to do is make the tests more tolerant of the new behaviour if the behaviour makes sense, or report the change to Last.fm if it doesn't. I haven't looked in detail but it feels like the tests should be able to cope with these changes.

With that in mind, I'm happy to merge this PR! Thanks again for all the work. I'm hoping to publish a release soon - I've just come out of a busy phase at work so hopefully will have a bit of time one of these evenings. Cheers