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: Catch JSONException from parsing vardata response #33

Merged
merged 7 commits into from
Sep 26, 2023

Conversation

tyiuhc
Copy link
Collaborator

@tyiuhc tyiuhc commented Sep 26, 2023

No description provided.

@tyiuhc tyiuhc requested a review from bgiori September 26, 2023 18:34
}
} catch (e: JSONException) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should catch this exception from the place where parseResponse is called rather than inside this function. Line 238. Then we can call the onFailure function to complete the future exceptionally and propagate the error more clearly.

@tyiuhc tyiuhc requested a review from bgiori September 26, 2023 18:54
Comment on lines 239 to 242
when (e) {
is IOException -> onFailure(call, e)
is JSONException -> future.completeExceptionally(e)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

onFailure is the same as calling completeExceptionally, so we should just maintain consistency. Further, this logic would ignore all other errors, which is not ideal. Either we (a) catch and propagate all exceptions or (b) only catch and propagate some exceptions, leavin some to be thrown in the callback.

I think we go with (a) so we can just do catch any exception and pass it to onFailure

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On this, the onFailure function in the Callback interface has signature fun onFailure(call: Call, e: IOException), should I create an onFailure function which takes in any exception type then?

@tyiuhc tyiuhc requested a review from bgiori September 26, 2023 19:48
@tyiuhc tyiuhc merged commit cabbb5d into main Sep 26, 2023
4 checks passed
@tyiuhc tyiuhc deleted the parse-response-catch branch September 26, 2023 19:59
github-actions bot pushed a commit that referenced this pull request Sep 26, 2023
## [1.10.1](1.10.0...1.10.1) (2023-09-26)

### Bug Fixes

* Catch JSONException from parsing vardata response ([#33](#33)) ([cabbb5d](cabbb5d))
@github-actions
Copy link

🎉 This PR is included in version 1.10.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants