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

Fix: we should be able to handle a "null" result from a server #458

Merged
merged 3 commits into from Nov 10, 2018

Conversation

rwols
Copy link
Member

@rwols rwols commented Nov 9, 2018

If there's an error response, handle that.
Otherwise, handle the result.

If there's an error response, handle that.
Otherwise, handle the result.
@coveralls
Copy link

coveralls commented Nov 9, 2018

Coverage Status

Coverage increased (+0.04%) to 28.259% when pulling 9a5262f on bugfix/handle-none-result into 03acb77 on master.

Copy link
Contributor

@tomv564 tomv564 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to have more tests!
I'm a bit nervous that we're now moving to a 3rd way of handling responses instead of reverting to the original result/error checking.
Especially since it wasn't needed to get your memory leak fix in place?
If we continue with this logic, why did we drop the "invalid response payload" case?

…error' keys are present, as well as when both are missing
@rwols
Copy link
Member Author

rwols commented Nov 10, 2018

If we continue with this logic, why did we drop the "invalid response payload" case?

the logic seemed somewhat complex:

no error key, no result key ==> invalid response
error key, no result key ==> handle error
no error key, result key ==> handle result
error key, result key ==> invalid response

I thought: If there's an error key present we should handle it regardless of the result key. Because of that, it turned out that the "invalid response" logical path went away.

Especially since it wasn't needed to get your memory leak fix in place?

You're right, I've reverted to the old behavior and added unit tests for that behavior.

@tomv564 tomv564 merged commit eb5e247 into master Nov 10, 2018
@tomv564
Copy link
Contributor

tomv564 commented Nov 10, 2018

Thanks for the quick fix!

@rwols rwols deleted the bugfix/handle-none-result branch November 10, 2018 11:22
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.

None yet

3 participants