Non-success status codes should not always result in a thrown exception #30
Conversation
…t automatically throw an exception
| [nubank/state-flow "5.14.0"] | ||
| [clj-commons/fs "1.6.310"] | ||
| [nubank/state-flow "5.17.0"] | ||
| [clj-commons/fs "1.6.311"] |
There was a problem hiding this comment.
Are the version bumps essential to this PR? Otherwise separate them out at least to a separate commit.
There was a problem hiding this comment.
When the cost of getting a commit merged is too high, you tend to put a lot into one PR. These changes are not essential, but I tend to update dependencies as hygiene, when possible.
src/clj_github/httpkit_client.clj
Outdated
|
|
||
| :throw? - if true (the default), then non-success status codes result in a thrown exception. | ||
| If set to false, then the response is returned, regardless and the caller can decide what to do | ||
| with failure statuses." |
There was a problem hiding this comment.
💯 for adding better docs.
Maybe specific what non-success status codes are, just to be sure. You can refer to the success-codes function.
Also add that in case of success the response body is conditionally parsed into json as returned as a clojure data structure.
There was a problem hiding this comment.
Adding docs is what I do ™️.
| {:status 404}] | ||
| (is (match? | ||
| {:status 404} | ||
| (sut/request client {:throw? false}))))) |
There was a problem hiding this comment.
You should also verify that the headers and body are not mangled.
|
Thanks for the review; I'll make some changes and merge. |
There are cases where a valid request to GitHub may result in a 404, and dealing with that as a thrown exception, rather than a status code, is problematic.
Also, bumped many deps to latest, except where that causes a conflict with JDK 1.8.