search

adamkrogh / goodreads-dotnet

:books: Goodreads .NET API Client Library
MIT License
51 stars 15 forks source link

Added support for adding book to shelf. #2

Closed VladimirRybalko closed 7 years ago

VladimirRybalko commented 7 years ago

Hi @adamkrogh There is my first pull request. I decide to develop the "Add book to shelf" method. Please review this and let me know if you find any issues.

Also I should note one thing. I've added an addition row into into RestSharpExtensions.cs file. if (!string.IsNullOrEmpty(message))

It's important because "shelf/add_to_shelf" API method returns a hash structure as a correct response. For example: `

to-read removed

`

So I need a way to distinguish the correct and failed response.

adamkrogh commented 7 years ago

Hey! Thanks for the pull request, it looks awesome! I'll have a chance to review it tonight after work.

adamkrogh commented 7 years ago

Just tried your changes out, the tests you have for adding and removing a book from a shelf work great. Nice!

Adding that change to the RestSharpExtensions makes sense. The Goodreads API is very inconsistent so there's bound to be a lot of differences on the rest of the API calls.

One thing I thought I'd mention is that I enabled some code analysis/linting in the project using StyleCop. You can notice some warnings in the "Error List" window of Visual Studio when building the project. These are mainly just to keep the code style consistent and to follow some best practices. You can check them over periodically to see if there's some suggestions to clean up the code. I know I might have left some unfixed from my last changes so it's not too big of a deal.

Also, I'm not sure why the build is failing. I'll have to look into that soon too but you can ignore it for now.

Everything else looks awesome, I'll merge this right now!

VladimirRybalko commented 7 years ago

I have seen that CI server shows the failed build. I've check it again and think that it relates with integration tests. You are using enviroment varibales to set auth tokens. Seems these variables were not configured on CI server.

adamkrogh commented 7 years ago

You're right they are defined by environment variables but I do have them setup on the CI server.

I have a test profile on Goodreads (here) that some of the tests run on and I think I might have accidentally deleted one of the reviews that are being checked by an integration test.

I also noticed that TheGetListByUserMethod.ReturnsSortedList test in ReviewsClientTests is actually failing because Goodreads isn't returning a correctly sorted list... I might have to disable that one if their API isn't working correctly.

adamkrogh commented 7 years ago

Looks like that was it, I must have deleted a review under test. Guess that's the problem with integration tests on real profiles! I also commented out one assertion on that ReturnsSortedList test since Goodreads isn't returning the correct response. All the other tests pass though and the build is back to normal.