Closed PoLaKoSz closed 5 years ago
Thanks to submitting this. I'll review it over the next few days (as I'm away) and get back to you! :)
I added an another commit that i forgot yesterday which adds more live API call tests (Add and Remove from Favourites - Artist, Track, Album, Playlist and Radio). I wasn't able to test what property change when i rate an album, so i didn't added a test for it and also i didn't added test to post a comment because it maybe trigger a notification and i don't want to spam other users with notifications.
With this the failing tests count increase to 46 but with the fixes it will increase the code coverage too with additional ~5%.
I wanted to ask should i commit the fixes here too? It contains two breaking change:
Album
's GenreId
type changes from uint
to int
BrowseEndpoint
's GetUserById
return type changes from Task<IUser>
to Task<IUserProfile>
. I changed the type because the return json is more similar to this type compared to the IUser
.
I want to mention that i added a new property called Country
to the IUserProfile
because a GET
request to the https://api.deezer.com/user/5 URL will return a country
property too but maybe this is not the best option because i saw for example the https://api.deezer.com/album/302127/fans will not contains the country
property. What do You think about this?Did You have a chance to take a look at it?
- I'd prefer if there was a consistent coding style throughout the project. I'm not overly happy with the style used originally and have been slowly changing bits as I touch the code. I'll come up with a style and note it somewhere for anyone wishing to contribute in the future.
Usually just temporary but i often use a Microsoft tool called StyleCop to address this issue. It has a great documentation which can help to modify the default rules (for ex.:
I wanted to ask should i commit the fixes here too? It contains two breaking change:
Album
's GenreId
type changes from uint
to int
BrowseEndpoint
's GetUserById
return type changes from Task<IUser>
to Task<IUserProfile>
. I changed the type because the return json is more similar to this type compared to the IUser
.
I want to mention that i added a new property called Country
to the IUserProfile
because a GET
request to the https://api.deezer.com/user/5 URL will return a country
property too but maybe this is not the best option because i saw for example the https://api.deezer.com/album/302127/fans will not contains the country
property. What do You think about this?
Probably this is a huge commit but it does not contain any breaking changes (only adds more test cases to the current Unit test project).
Additionally I added two different type of tests called
Integration
andRegression
. The Integration tests just loads a previously saved API response and tries to parse it, while the Regression tests calls directly the live API.I want to mention that this commit will introduce 28 failing tests (but increase the code coverage from ~41% to ~68%). I already implemented the fixes for them but i thought i will wait for some feedback first.
Ref.: #26