Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed GetRawResponseContent to return content if there is some rega… #61

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

westwok
Copy link

@westwok westwok commented Jan 19, 2016

…rdless of HttpStatusCode

@westwok
Copy link
Author

westwok commented Jan 27, 2016

Return more information to the developer when there's a JsonSerializationException. If it was a 404 you would not know otherwise.

@westwok westwok closed this Jan 27, 2016
@westwok westwok reopened this Jan 27, 2016
@sahb1239
Copy link

sahb1239 commented Feb 8, 2016

I have faced same issue but returning null should not be needed? (I know the tests are expecting null, but this can be changed to asserting string.IsNullOrEmpty() - this could however be a problem with compatibility)

@westwok
Copy link
Author

westwok commented Feb 9, 2016

I suppose returning null will maintain compatibility and, if there is no content, it will behave as it did before. Are there any alternatives that would not break compatibility?

@sahb1239
Copy link

You are breaking compatibility under all circumstances by returning data where it before returned null. The pull request are also breaking compatibility for success status codes since now the libary returns null where it before returned a empty string.

I really don't think that the users of this libary is checking for null to determine if a request has failed before checking the statuscode.

So I really think you should be consistent and just returing the response and change the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants